When should I use temporary variables?

Question

Specifically, I'm wondering which of these I should write:

{
    shared_ptr<GuiContextMenu> subMenu = items[j].subMenu.lock();
    if (subMenu)
        subMenu->setVisible(false);
}

or:

{
    if (items[j].subMenu.lock())
        items[j].subMenu.lock()->setVisible(false);
}

I am not required to follow any style guidelines. After optimization, I don't think either choice makes a difference in performance. What is generally the preferred style and why?

EDIT: the type of items[j].subMenu is boost::weak_ptr. lock() creates a shared_ptr out of it. There is actually an ambiguous difference in the two versions above, regarding how long the temporary shared_ptr lasts, so I wrapped my two examples in { braces } to resolve the ambiguity there.

Answer 1

I think you're correct about either choice being no different after optimisation.

Personally, I would declare a new variable if it makes the code more readable, such as when you're chaining calls, or putting function calls inside function calls. As long as it's maintainable and the code achieves the same effect at no speed difference, it all boils down to readable code.

Edit:

mmyers bought up a good comment. Yes, be careful about calling lock() twice, as opposed to just once. They will have different effects depending on your implementation.

Answer 2

I prefer the first one most of the time because it makes the code more clear and easy to read, therefore less error prone. For example, you forgot a parenthesis on that second example :) In this case, actually, I'd probably do what you did in the second example, however if I needed to use that submenu more than a few times I'd go with the first one to make the code easier to read. As for performance, I thing any sane compiler would be able to optimize that (which is probably why you saw no difference in performance).

Also, as mmyers pointed out, that also depends on what lock() does. In general, if it's a simple getter method or something like that, you'll be fine.

Answer 3

Whatever YOU prefer. For me, it depends on how much I'll use it; for two lines, I might just write it out both times, whereas I create a variable if I use it more. However, YOU are the one who will most likely have to maintain this code and continue looking at it, so use whatever works for you. Of course, if you're at a company with a coding guideline, follow it.

Answer 4

The choice is essentially up to you, but the basic thing you should look out for is maintainability.

Answer 5

In this specific example, I think it depends on what lock() does. Is the function expensive? Could it return different things each time the function is called (could it return a pointer the first time and NULL the second time)? Is there another thread running that could interleave between the two calls to lock()?

For this example, you need to understand the behavior of lock() and the rest of your code to make an intelligent decision.

Answer 6

When the return value is anything other that a boolean, assigning it to an intermediate variable can often simplify debugging. For example, if you step over the following:

if( fn() > 0 ) ...

all you will know after the fact was that the function returned a value either less than zero, or zero or more. Even if the return value were incorrect, the code may still appear to work. Assigning it to a variable that can be inspected in your debugger will allow you to determine whether the return value was expected.

When the return is boolean, the actual value is entirely implicit by the code flow, so it is less critical; however under code maintenance you may find later you need that result, so you may decide to make it a habit in any case.

Even where the return value is boolean, another issue to consider is whether the function has required side-effects, and whether this may be affected by short-circuit evaluation. For example in the statement:

if( isValid && fn() ) ...

the function will never be called is isValid is false.

The circumstances under which the code could be broken under maintenance by the unwary programmer (and it is often the less experienced programmers that get the maintenance tasks) are many, and probably best avoided.

Answer 7

An alternative method:

if(shared_ptr<GuiContextMenu> subMenu = items[j].subMenu.lock()) {
    subMenu->setVisible(false);
}
//subMenu is no longer in scope

I'm assuming subMenu is a weak_ptr, in which case your second method creates two temporaries, which might or might not be an issue. And your first method adds a variable to a wider scope than it needs to. Personally, I try to avoid assignments within if statements, but this is one of the few cases where I feel its more useful than the alternatives.

Answer 8

I think the preferred style is whatever style you think makes your code more readable and maintainable. If you're a team of more than one, the only other consideration is that it's generally a good idea for everyone to adopt the same style, again for readability and ease of maintenance.

Answer 9

In this particular case, you really should use the version with the temporary variable. The reason is not performance, but correctness - basically, you are not guaranteed that the two x.lock() calls return the same value (eg. if another thread releases the last strong reference on the object just between the two calls). By holding the strong reference in the temporary variable, you ensure it won't go away.

Other than that:

• the compilers usually can't optimise out function calls, unless they are provably side-effect free (this is hard to do, but attributes may help) or inlined. In this case, the call has side-effects.

• using temporaries can lead to shorter, more readable and more maintainable programs (eg. in case of error, you fix it in one place)

Answer 10

In this case I think you should use the temporary. Even if you know the implementation to .lock() is inexpensive, that can change. If you don't need to call lock() twice, don't. The value here is that it decouples your code from the implementation of lock(). And that's a good thing generally.