views:

267

answers:

8

When I find myself calling the same getter method multiple times, should this be considered a problem? Is it better to [always] assign to a local variable and call only once?

I'm sure the answer of course is "it depends".

I'm more concerned about the simpler case where the getter is simply a "pass-along-the-value-of-a-private-variable" type method. i.e. there's no expensive computation involved, no database connections being consumed, etc.

My question of "is it better" pertains to both code readability (style) and also performance. i.e. is it that much of a performance hit to have:

SomeMethod1(a, b, foo.getX(), c);
SomeMethod2(b, foo.getX(), c);
SomeMethod3(foo.getX());

vs:

X x = foo.getX();
SomeMethod1(a, b, x, c);
SomeMethod2(b, x, c);
SomeMethod3(x);

I realize this question is a bit nit-picky and gray. But I just realized, I have no consistent way of evaluating these trade-offs, at all. Am fishing for some criteria that are more than just completely whimsical.

Thanks.

+2  A: 

Most of the time I would use getX if it was only once, and create a var for it for all other cases. Often just to save typing.

With regards to performance, the compiler would probably be able to optimize away most of the overhead, but the possibility of side-effects could force the compiler into more work when doing multiple method-calls.

svrist
+7  A: 

For plain getters - those that just returns a value - HotSpot inlines it in the calling code, so it will be as fast as it can be.

I, however, have a principle about keeping a statement on a single line, which very often results in expressions like "foo.getBar()" being too long to fit. Then it is more readable - to me - to extract it to a local variable ("Bar bar = foo.getBar()").

Thorbjørn Ravn Andersen
+6  A: 

They could be 2 different things.

If GetX is non-deterministic then the 1st one will give different results than the 2nd

Personally, I'd use the 2nd one. It's more obvious and less unnecessarily verbose.

gbn
The word you're looking for is "idempotent". `x+=2; return x;` is completely deterministic, but will give different results when called multiple times.
Michael Madsen
@Michael Madsen: true. sloppy thinking on my part
gbn
+12  A: 

The choice shouldn't really be about performance hit but about code readability.

When you create a variable you can give it the name it deserves in the current context. When you use a same value more than one time it has surely a real meaning, more than a method name (or worse a chain of methods).
And it's really better to read:

String username = user.getName();
SomeMethod1(a, b, username, c);
SomeMethod2(b, username, c);
SomeMethod3(username);

than

SomeMethod1(a, b, user.getName(), c);
SomeMethod2(b, user.getName(), c);
SomeMethod3(user.getName());
Colin Hebert
IMO, `user.getName()` is sometimes easier to read, because you don't have to search (and scroll up) to the position where it's been set. You'd probably name the variable `username` too, if it were set via `request.getUsername()` - so which one is it?
Chris Lercher
+1 for code readability
extraneon
@chris_l like I said each variable name has its own meaning in the context where it's used. And yes, I would probably name `request.getUsername()` username too. And if I had to use both, there would be a reason, like a new username and an old one. newUsername, oldUsername could fit. It's all about the context.
Colin Hebert
@Colin: That's true (and BTW, I'd do the same), but when you're scrolled down to the end of the method, you don't see the context immediately. So when you just see `username`, you don't know exactly what it is at first.
Chris Lercher
Java method names should start with lowercase.
Steve Kuo
I just copy/pasted them from the question.
Colin Hebert
@chris_l It's pretty much guaranteed that *user* was set before *username* was, so you'd have to scroll up even further to understand the context that *user.getName()* is being invoked in. As such, the definition of *username* is going to be closer to the code that uses it, then that of *user*.
Vladislav
@Vladislav: We're back to "it depends", because it really does (and that's why I said that it's "sometimes" easier). Often, you don't have to *understand* the context. All you want, is to understand a few lines of code very quickly (say 10 seconds). After that, you can scroll up to reassure, that you've understood the context. The thing that's really hard is, when you have to scroll up and down again and again (or worse: waste short-time memory to remember multiple variable names), before you even have a feeling what these few lines do.
Chris Lercher
+3  A: 

I use the second style if it makes my code more readable or if I have to use the assigned value again. I never consider performance (on trivial things) unless I have to.

stacker
Plus it may be easier to set a breakpoint to X x = foo.getX(); to examine the value returned.
RealHowTo
+3  A: 

That depends on what getX() actually does. Consider this class:

public class Foo {
    private X x;

    public X getX() { return x; }
}

In this case, when you make a call to foo.getX(), JVM will optimize it all the way down to foo.x (as in direct reference to foo's private field, basically a memory pointer). However, if the class looks like this:

public class Foo {
    private X x;

    public X getX() { return cleanUpValue(x); }

    private X cleanUpValue(X x) {
        /* some modifications/sanitization to x such as null safety checks */
    }
}

the JVM can't actually inline it as efficiently anymore since by Foo's constructional contract, it has to sanitize x before handing it out.

To summarize, if getX() doesn't really do anything beyond returning a field, then there's no difference after initial optimization runs to the bytecode in whether you call the method just once or multiple times.

Esko
If I have understood it correctly, HotSpot can inline the getX() even if it calls more code. The decision is based on the size of the method - not what it actually does.
Thorbjørn Ravn Andersen
I seem to remember reading in some stuff on jruby that Hotspot currently inlines up to 9 nested calls
Gareth Davis
The JVM should have no problem inline getX and cleanUpValue.
Steve Kuo
Follow this method, and you're programming against the underlying implementation, while you _should_ be programming against the interface.
xtofl
What if Foo has a public setX method? (Especially if the application is multi-threaded.) Even if getX just returns a field, the field itself might change and calling the method multiple times may return different values.
emory
@emory: Being essentially just a memory pointer eventually after all the JIT runs and optimization, that doesn't matter - assuming of course that there's no synchronization between the methods or explicit returnal of a copy in the accessor method (`getX()`).
Esko
+1  A: 

Two things have to be considered:

  1. Does the call to getX() have any side effects? Following established coding patterns, a getter should not alter the object on which it is called, the in most cases, there is no side effect. Therefore, it is semantically equivalent to call the getter once and store the value locally vs. calling the getter multiple times. (This concept is called idempotency - it does not matter whether you call a method once or multiple times; the effect on the data is exactly the same.)

  2. If the getter has no side effect, the compiler can safely remove subsequent calls to the getter and create the temporary local storage on its own - thus, the code remains ultra-readable and you have all the speed advantage from calling the getter only once. This is all the more important if the getter does not simply return a value but has to fetch/compute the value or runs some validations.

Assuming your getter does not change the object on which it operates it is probably more readable to have multiple calls to getX() - and thanks to the compiler you do not have to trade performance for readability and maintainability.

Martin Eisenhardt
+1  A: 

I generally store it locally if:

  1. I'm will use it in a loop and I don't want or expect the value to change during the loop.

  2. I'm about to use it in a long line of code or the function & parameters are very long.

  3. I want to rename the variable to better correspond to the task at hand.

  4. Testing indicates a significant performance boost.

Otherwise I like the ability to get current values and lower level of abstraction of method calls.

Thomas Langston