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.)

Solution 1

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.

Solution 2

Starting Java 14 you should be able to do instanceof and cast at the same time. See https://openjdk.java.net/jeps/305.

Code example:

if (obj instanceof String s) {
    // can use s here
} else {
    // can't use s here
}

The variable s in the above example is defined if instanceof evaluates to true. The scope of the variable depends on the context. See the link above for more examples.

Solution 3

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.

Solution 4

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.

Solution 5

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;
    }
}

View more solutions

Comments

[Nikita Rybak]

I would use instanceOf instead of class.cast to avoid unnecessary exceptions in castOrNull, but apart from that you seem to have perfectly normal helper. Let Tom clarify what he meant.

[Péter Török]

If Y is a superclass of X, you need no cast at all - did you mean subclass?

[Nikita Rybak]

Well, there're many libraries in Java which we don't design, but have to use. (Mentioned iterator being the one example)

[Péter Török]

@Nikita, fair enough, if you use a badly designed but at the same time indispensable Java library, you hardly have options. My personal guess is that such libraries should be very rare though. If a library is this awkward to use, then either noone uses it, or sooner or later someone starts to develop a better alternative.

[Albert]

I doubt that he meant that because I asked for better code and the code which I have posted here was his own suggestion.

[Péter Török]

A similar alternative was discussed in the comments to the linked answer, and Tom did not prefer it over the above variant.

[Thomas]

@Albert: missed that...since that's the case, I would say that most likely it is as Péter's answer says, a question of suboptimal design. But really, in order to figure out exactly why he believes it wrong to do, you have to wait for his response (if he ever gets back to you).

[Albert]

I have extended my question a bit to show 2 cases where I sometimes use it. I don't really like hard rules to really never ever use something and I think my provided two cases are valid cases where you could do it like this. You might be more extreme about this than me but I guess you would also not be so extreme to never ever use code like this. And then, if you happen to have such a case, I always like to have the most clean solution and that was what I am asking here about. :)

[Albert]

Ah, thanks for that information! So you would also argument that I should not use castOrNull? Or how would you value those contra-points against my given pro-points? (Because after all, avoiding a dummy variable can also lead to more readable and shorter code.)

[Stephen C]

The only pro point that I can discern is that is avoids using a temporary variable in some cases. For me, that is a purely cosmetic issue. Use of a temporary doesn't make the code any less readable IMO, and shorter code is not a readability advantage either. (And BTW it is not a "dummy" variable because it is used.)

[Péter Török]

@Albert, I don't like to say "never" either, especially here on SO ;-) I have been working with legacy code most of my career, so I had to learn to be pragmatic about living with suboptimal solutions. Using instanceof is a standard idiom, easy to understand to anyone, and - as Stephen C pointed out - easy to optimize. And I personally have no issues with temporary variables.