java: combined instanceof and cast ?

Question

(Please no advise that I should abstract X more and add another method to it.)

In C++, when I have a variable x of type X* and I want to do something specific if it is also of type Y* (Y being a subclass of X), I am writing this:

if(Y* y = dynamic_cast<Y*>(x)) {
    // now do sth with y
}

The same thing seems not possible in Java (or is it?).

I have read this Java code instead:

if(x instanceof Y) {
    Y y = (Y) x;
    // ...
}

Sometimes, when you don't have a variable x but it is a more complex expression instead, just because of this issue, you need a dummy variable in Java:

X x = something();
if(x instanceof Y) {
    Y y = (Y) x;
    // ...
}
// x not needed here anymore

(Common thing is that something() is iterator.next(). And there you see that you also cannot really call that just twice. You really need the dummy variable.)

You don't really need x at all here -- you just have it because you cannot do the instanceof check at once with the cast. Compare that again to the quite common C++ code:

if(Y* y = dynamic_cast<Y*>( something() )) {
    // ...
}

Because of this, I have introduced a castOrNull function which makes it possible to avoid the dummy variable x. I can write this now:

Y y = castOrNull( something(), Y.class );
if(y != null) {
    // ...
}

Implementation of castOrNull:

public static <T> T castOrNull(Object obj, Class<T> clazz) {
    try {
        return clazz.cast(obj);
    } catch (ClassCastException exc) {
        return null;
    }
}

Now, I was told that using this castOrNull function in that way is an evil thing do to. Why is that? (Or to put the question more general: Would you agree and also think this is evil? If yes, why so? Or do you think this is a valid (maybe rare) use case?)

---

As said, I don't want a discussion whether the usage of such downcast is a good idea at all. But let me clarify shortly why I sometimes use it:

1. Sometimes I get into the case where I have to choose between adding another new method for a very specific thing (which will only apply for one single subclass in one specific case) or using such instanceof check. Basically, I have the choice between adding a function doSomethingVeryVerySpecificIfIAmY() or doing the instanceof check. And in such cases, I feel that the latter is more clean.

2. Sometimes I have a collection of some interface / base class and for all entries of type Y, I want to do something and then remove them from the collection. (E.g. I had the case where I had a tree structure and I wanted to delete all childs which are empty leafs.)

Answer 1

I don't know exactly why the person said that it was evil. However one possibility for their reasoning was the fact that you were catching an exception afterwards rather than checking before you casted. This is a way to do that.

public static <T> T castOrNull(Object obj, Class<T> clazz) {
    if ( clazz.isAssignableFrom(obj.getClass()) ) {
        return clazz.cast(obj);
    } else {
        return null;
    }
}

Answer 2

IMHO your castOrNull is not evil, just pointless. You seem to be obsessed with getting rid of a temporary variable and one line of code, while to me the bigger question is why you need so many downcasts in your code? In OO this is almost always a symptom of suboptimal design. And I would prefer solving the root cause instead of treating the symptom.

Answer 3

Now, I was told that using this castOrNull function in that way is an evil thing do to. Why is that?

I can think of a couple of reasons:

• It is an obscure and tricky way of doing something very simple. Obscure and tricky code is hard to read, hard to maintain, a potential source of errors (when someone doesn't understand it) and therefore evil.

• The obscure and tricky way that the castOrNull method works most likely cannot be optimized by the JIT compiler. You'll end up with at least 3 extra method calls, plus lots of extra code to do the type check and cast reflectively. Unnecessary use of reflection is evil.

(By contrast, the simple way (with instanceof followed by a class cast) uses specific bytecodes for instanceof and class casting. The bytecode sequences can almost certainly will be optimized so that there is no more than one null check and no more that one test of the object's type in the native code. This is a common pattern that should be easy for the JIT compiler to detect and optimize.)

Of course, "evil" is just another way of saying that you REALLY shouldn't do this.

Neither of your two added examples, make use of a castOrNull method either necessary or desirable. IMO, the "simple way" is better from both the readability and performance perspectives.

Answer 4

In most well written/designed Java code the use of instanceof and casts never happens. With the addition of generics many cases of casts (and thus instanceof) are not needed. They do, on occasion still occur.

The castOrNull method is evil in that you are making Java code look "unnatural". The biggest problem when changing from one language to another is adopting the conventions of the new language. Temporary variables are just fine in Java. In fact all your method is doing is really hiding the temporary variable.

If you are finding that you are writing a lot of casts you should examine your code and see why and look for ways to remove them. For example, in the case that you mention adding a "getNumberOfChildren" method would allow you to check if a node is empty and thus able to prune it without casting (that is a guess, it might not work for you in this case).

Generally speaking casts are "evil" in Java because they are usually not needed. Your method is more "evil" because it is not written in the way most people would expect Java to be written.

That being said, if you want to do it, go for it. It isn't actually "evil" just not "right" way to do it in Java.