Java coding style, local variables vs repeated method calls.

Question

I prefer to use local variables rather than multiple calls to the same method.

/*
 * I prefer this
 */
Vehicle vehicle = person.getVehicle()
if (vehicle instanceof Car) {
   Car car = (Car) vehicle;
   car.openSunroof();
} else if (vehicle instanceof Bike) {
   Bike bike = (Bike) vehicle;
   bike.foldKickstand();
}
/*
 * Rather than this
 */
if (person.getVehicle() instanceof Car) {
   Car car = (Car) person.getVehicle();
   car.openSunroof();
} else if (person.getVehicle() instanceof Bike) {
   Bike bike = (Bike) person.getVehicle();
   bike.foldKickstand();
}

• I believe that the first way is going to perform a tiny bit faster
• I think the second way violates the DRY principle
• I find the first way more readable and easier to debug (... OK negligible because I could step over)
• I don't want to have to deal with the possibility of changed object state

Which do you prefer and why?

Answer 1

I prefer the first version for all the reasons you've mentioned. In particular (just to spell out your fourth point), it means you're definitely going to get consistent results... you could get horribly nasty results with the second version if getVehicle() returned a Car on the first call, then a Bike on the second...

The performance side doesn't bother me (I'll happily call List.size() repeatedly, for example) but readability, consistency and non-repeating are all much more important. Essentially the first snippet conveys the idea of "get a value, then use it" much more effectively than the second does.

So yes, I'm with you... is anyone recommending the second form to you?

Answer 2

I agree, but I also try to reduce use of 'instanceof' too, at the class design level.

Answer 3

I personally think the first one is cleaner. However, provided the called method is not something computation intensive, it doesn't matter much.

Probably the second is a bit faster (if you use Java 1.6) because in the first example you make a copy of the variable, while the java VM will be likely to inline the function call in both examples. Of course optimization is never an argument with calls like this. The compiler does so many optimizations that we shouldn't bother (often it'll just reduce the speed because we dont' know it well enough).

Answer 4

Yeah the first one is definitely better. I would never go for the second method. But you should think of using polymorphism more. Relying on instanceof so heavyly is not good OO design.

Answer 5

I normally dislike the introduction of extra variables as every bit of added state makes a method more complex. But even I'd say that it's warranted in your example since the variable replaces 4 repetitions of identical code.

But the variable should definitely be final!

Answer 6

As everybody that answered this question so far, I definitely prefer the first style. It can be even cleaner though:

Vehicle vehicle = person.getVehicle()
if (vehicle instanceof Car) {
   ((Car) vehicle).openSunroof();
} else if (vehicle instanceof Bike) {
   ((Bike) vehicle).foldKickstand();
}

Answer 7

Both examples need some work. Try to push the behavior into an abstract (or protected) method on Vehicle. If this is code you can't modify, use composition to put it inside of an interface in your code base so that you don't have to pollute the rest of your code with the poor design of the library you're using. This is definitely a code smell. See "Replace Conditional with Polymorphism" in Fowler's Refactoring book.