views:

256

answers:

7

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?

+7  A: 

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?

Jon Skeet
I believe Martin Fowler recommends the second form to make refactoring easier. (In my opinion refactoring should make the code easier to follow, not harder.)
Tom Hawtin - tackline
@Jon, would you do the same thing with a property in C# ?
finnw
I saw some proponents of the getter method calls on this thread http://stackoverflow.com/questions/1923795/java-method-invocation-vs-using-a-variable
crowne
+1  A: 

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

Jim Blackler
+1  A: 

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

Thirler
+2  A: 

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.

fastcodejava
Nice point, that could make it even cleaner. But then you might be looking for a common interface, such as prepareForJourney() which could be implmented separately in each class. However, we are often working with 3rd party or legacy classes which may not lend themselves to easily applying a new interface, but I suppose they could be wrapped, and apply the interface to the wrapper.
crowne
+2  A: 

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!

Michael Borgwardt
+1 for `final`.
uckelman
+1  A: 

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();
}
Bruno Rothgiesser
+1  A: 

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.

Rob Thornton