Function calls vs. local variables

Question

I often see functions where other functions are called multiple times instead of storing the result of the function once.

i.e (1):

void ExampleFunction()
{ 
    if (TestFunction() > x || TestFunction() < y || TestFunction() == z)
    {
        a = TestFunction();
        return; 
    }
    b = TestFunction();
}

Instead I would write it that way, (2):

void ExampleFunction()
{
    int test = TestFunction();
    if (test > x || test < y || test == z)
    {
        a = test;
        return;
    }
    b = test;
}

I think version 2 is much better to read and better to debug. But I'm wondering why people do it like in number 1? Is there anything I don't see? Performance Issue? When I look at it, I see in the worst case 4 function calls in number (1) instead of 1 function call in number (2), so performance should be worse in number (1), shouldn't it?

Answer 1

I'd use (2) if I wanted to emphasize that the same value is used throughout the code, or if I wanted to emphasize that the type of that value is int. Emphasizing things that are true but not obvious can assist readers to understand the code quickly.

I'd use (1) if I didn't want to emphasize either of those things, especially if they weren't true, or if the number of times that TestFunction() is called is important due to side-effects.

Obviously if you emphasize something that's currently true, but then in future TestFunction() changes and it becomes false, then you have a bug. So I'd also want either to have control of TestFunction() myself, or to have some confidence in the author's plans for future compatibility. Often that confidence is easy: if TestFunction() returns the number of CPUs then you're happy to take a snapshot of the value, and you're also reasonably happy to store it in an int regardless of what type it actually returns. You have to have minimal confidence in future compatibility to use a function at all, e.g. be confident that it won't in future return the number of keyboards. But different people sometimes have different ideas what's a "breaking change", especially when the interface isn't documented precisely. So the repeated calls to TestFunction() might sometimes be a kind of defensive programming.

Answer 2

When a temporary is used to store the result of a very simple expression like this one, it can be argued that the temporary introduces unecessary noise that should be eliminated.

In his book "Refactoring: Improving the Design of Existing Code", Martin Fowler lists this elimination of temporaries as a possibly beneficial refactoring (Inline temp).

Whether or not this is a good idea depends on many aspects:

• Does the temporary provides more information than the original expression, for example through a meaningful name?
• Is performance important? As you noted, the second version without temporary might be more efficient (most compilers should be able to optimize such code so that the function is called only once, assuming it is free of side-effects).
• Is the temporary modified later in the function? (If not, it should probably be const)
• etc.

In the end, the choice to introduce or remove such temporary is a decision that should be made on a case by case basis. If it makes the code more readable, leave it. If it is just noise, remove it. In your particular example, I would say that the temporary does not add much, but this is hard to tell without knowing the real names used in your actual code, and you may feel otherwise.