2015-04-30

Wow, 5 upvotes, 10 downvotes and counting

I have this answer on programmers.stackexchange.com which, at the time of writing these words, has 5 upvotes and 10 downvotes, and in all likelihood it will continue collecting downvotes, while I adamantly refuse to remove it, standing 100% by my ideas. I am dumbfounded, as such a thing has never happened before.

Here is the programmers.stackexchange.com question:

Is it okay to have objects that cast themselves, even if it pollutes the API of their subclasses?


I have a base class, Base. It has two subclasses, Sub1 and Sub2. Each subclass has some additional methods. For example, Sub1 has Sandwich makeASandwich(Ingredients... ingredients), and Sub2 has boolean contactAliens(Frequency onFrequency).

Since these methods take different parameters and do entirely different things, they're completely incompatible, and I can't just use polymorphism to solve this problem.

Base provides most of the functionality, and I have a large collection of Base objects. However, all Base objects are either a Sub1 or a Sub2, and sometimes I need to know which they are.

It seems like a bad idea to do the following:

for (Base base : bases) {
    if (base instanceof Sub1) {
        ((Sub1) base).makeASandwich(getRandomIngredients());
        // ... etc.
    } else { // must be Sub2
        ((Sub2) base).contactAliens(getFrequency());
        // ... etc.
    }
}

So I came up with a strategy to avoid this without casting. Base now has these methods:

boolean isSub1();
Sub1 asSub1();
Sub2 asSub2();

And of course, Sub1 implements these methods as

boolean isSub1() { return true; }
Sub1 asSub1();   { return this; }
Sub2 asSub2();   { throw new IllegalStateException(); }

And Sub2 implements them in the opposite way.

Unfortunately, now Sub1 and Sub2 have these methods in their own API. So I can do this, for example, on Sub1.

/** no need to use this if object is known to be Sub1 */
@Deprecated
boolean isSub1() { return true; }

/** no need to use this if object is known to be Sub1 */
@Deprecated
Sub1 asSub1();   { return this; }

/** no need to use this if object is known to be Sub1 */
@Deprecated
Sub2 asSub2();   { throw new IllegalStateException(); }

This way, if the object is known to be only a Base, these methods are un-deprecated, and can be used to "cast" itself to a different type so I can invoke the subclass's methods on it. This seems elegant to me in a way, but on the other hand, I'm kind of abusing Deprecated annotations as a way to "remove" methods from a class.

Since a Sub1 instance really is a Base, it does make sense to use inheritance rather than encapsulation. Is what I'm doing good? Is there a better way to solve this problem?

Tags: java, inheritance, type-casting
asked by codebreaker


The highest scoring answer so far has collected 21 upvotes and 3 downvotes, and it dismisses the OPs question stating that his design is wrong.  Basically, it draws a straw man of the OP's description and proceeds to attack it.  The strawman is terrible, so he is mighty successful at destroying it. Needless to say, one of the 3 downvotes is mine.

Then, here is my answer, which turned out to be highly unpopular:
What you are doing is perfectly legitimate. Do not pay attention to the naysayers who merely reiterate dogma because they read it in some books. Dogma has no place in engineering.

I have employed the same mechanism a couple of times, and I can say with confidence that the java runtime could have also done the same thing in at least one place that I can think of, thus improving performance, usability, and readability of code that uses it.

Take for example java.lang.reflect.Member, which is the base of java.lang.reflect.Field and java.lang.reflect.Method. (The actual hierarchy is a bit more complicated than that, but that's irrelevant.) Fields and methods are vastly different animals: one has a value that you can get or set, while the other has no such thing, but it can be invoked with a number of parameters and it may return a value. So, fields and methods are both members, but the things you can do with them are about as different from each other as making sandwiches vs. contacting aliens.

Now, when writing code that uses reflection we very often have a Member in our hands, and we know that it is either a Method or a Field, (or, rarely, something else,) and yet we have to do all the tedious instanceof to figure out precisely what it is and then we have to cast it to get a proper reference to it. (And this is not only tedious, but it also does not perform very well.) The Method class could have very easily implemented the pattern that you are describing, thus making the life of thousands of programmers easier.

Of course, this technique is only viable in small, well-defined hierarchies of closely coupled classes that you have (and will always have) source-level control of: you don't want to be doing such a thing if your class hierarchy is liable to be extended by people who are not at liberty to refactor the base class.

Here is how what I have done differs from what you have done:
  • The base class provides a default implementation for the entire asDerivedClass() family of methods, having each one of them return null.
  • Each derived class only overrides one of the asDerivedClass() methods, returning this instead of null. It does not override any of the rest, nor does it want to to know anything about them. So, no IllegalStateExceptions are thrown.
  • The base class also provides final implementations for the entire isDerivedClass() family of methods, coded as follows: return asDerivedClass() != null; This way, the number of methods that need to be overriden by derived classes is minimized.
  • I have not been using @Deprecated in this mechanism because I did not think of it. Now that you gave me the idea, I will put it to use, thanks!
C# has a related mechanism built-in via the use of the as keyword. In C# you can say DerivedClass derivedInstance = baseInstance as DerivedClass and you will get a reference to a DerivedClass if your baseInstance was of that class, or null if it was not. This (theoretically) performs better than is followed by cast, (is is the admittedly better named C# keyword for instanceof,) but the custom mechanism that we have been hand-crafting performs even better: the pair of instanceof-and-cast operations of Java, as well as the as operator of C# do not perform as fast as the single virtual method call of our custom approach.

I hereby put forth the proposition that this technique should be declared to be a pattern and that a nice name should be found for it.

Gee, thanks for the downvotes!


A summary of the controversy, to save you from the trouble of reading the comments:

People's objection appears to be that the original design was wrong, meaning that you should never have vastly different classes deriving from a common base class, or that even if you do, the code which uses such a hierarchy should never be in the position of having a base reference and needing to figure out the derived class. Therefore, they say, the self-casting mechanism proposed by this question and by my answer, which improves the use of the original design, should never have been necessary in the first place. (They don't really say anything about the self-casting mechanism itself, they only complain about the nature of designs that the mechanism is meant to be applied to.)

However, in the example above I have already shown that the creators of the java runtime did in fact choose precisely such a design for the java.lang.reflect.Member, Field, Method hierarchy, and in the comments below I also show that the creators of the C# runtime independently arrived at an equivalent design for the System.Reflection.MemberInfo, FieldInfo, MethodInfo hierarchy. So, these are two different real world scenarios which are sitting right under everyone's nose and which have demonstrably workable solutions using precisely such designs.

That's what all the following comments boil down to. The self-casting mechanism is hardly mentioned.

So, I have copied all this here for posterity, because I really do stand by my convictions and I will continue to do so no matter how many people disagree with me, for as long as I don't see any valid arguments against my convictions.

Acceptance of some strategy and dismissal of some other strategy without objective reasons for doing so is just plain good ol' dogma.

3 comments:

  1. See your note on "Clean Code", Page 291 - “G7: Base Classes Depending on Their Derivatives” here: http://blog.michael.gr/2013/03/my-notes-on-clean-code.html. Seems you changed your tune by admitting there are exceptions. How... unpopular!

    ReplyDelete
  2. Yes, I changed my mind. There are some exceptions. I do have the right to change my mind, right? Have you never changed your mind about anything? How do you like it when people make snide remarks about changes of tune? Anyhow, thanks for pointing that out to me, I will now go fix it.

    ReplyDelete
    Replies
    1. So, I fixed it. The comment that I removed about Page 291 "G7" was: "A class should not know anything about its derivatives, period. I do not believe that there should ever be exceptions to this rule." Well, it turns out, there are some cases where this rule can have some useful exceptions.

      Delete