java: combined instanceof and cast?

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.

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.

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.