When should you use temporary variables?
Specifically, I'm wondering which one should I write from:
{
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 don't need to follow any style rules. After optimization, I don't think any choice matters in performance. Generally your preferred style and why?
EDIT: Item type [j] .subMenu is boost :: weak_ptr. lock () creates a shared_ptr from it. In fact, there is an ambiguous difference in the two versions above as to how long the temporary shared_ptr lasts, so I wrapped my two examples in {curly braces} to eliminate the ambiguity there.
a source to share
Alternative method:
if(shared_ptr<GuiContextMenu> subMenu = items[j].subMenu.lock()) {
subMenu->setVisible(false);
}
//subMenu is no longer in scope
I guess it subMenu
is weak_ptr
, in which case your second method creates two temporary files, which may or may not be the problem. And your first method adds the variable to a wider scope than it needs to. Personally, I try to avoid assignments in operators if
, but this is one of the few cases where I find it more useful than the alternatives.
a source to share
In this particular case, you should really use the temp version. The reason is not performance, but correctness - in principle, you are not guaranteed that two calls x.lock()
return the same value (for example, if another thread releases the last strong reference to an object only between two calls). By keeping a strong reference in a temporary variable, you make sure it doesn't disappear.
Besides:
-
compilers usually cannot optimize function calls unless they are explicitly free of side effects (difficult to do, but attributes can help) or inline. In this case, the call has side effects.
-
Using time series can lead to shorter, more readable and more maintainable programs (e.g., in case of error, fix them in one place)
a source to share
I think you are correct that any choice is no different after optimization.
Personally, I declare a new variable if it makes the code more readable, such as when you chaining calls or placing function calls inside function calls. As long as it is maintained and the code achieves the same effect with no difference in speed, it all boils down to readable code.
Edit:
mmyers were buying up a good comment. Yes, be careful when calling lock()
twice, not just once. They will have different effects depending on your implementation.
a source to share
In this particular example, I think it depends on what it does lock()
. Is the feature expensive? Can it return different things on every function call (can it return a pointer the first time and NULL the second time)? Is there another thread that can alternate between the two calls on lock()
?
In this example, you need to understand the behavior lock()
and the rest of your code in order to make an intelligent decision.
a source to share
When the return value is something else that is boolean, assigning it to an intermediate variable often makes debugging easier. For example, if you go to the following:
if( fn() > 0 ) ...
whatever you learn after the function returned a value of zero, or zero or more. Even if the return value was incorrect, the code might still work. Assigning a variable to a variable that can be checked in your debugger will allow you to determine if a return value was expected.
When the return is boolean, the actual value is completely implicit in the code flow, so it is less critical; however, when maintaining your code, you may find later that you want this result, so you can make it a habit anyway.
Even if the return value is boolean, another issue to consider is whether the functions need side effects, and whether this might affect the evaluation of the short circuit. For example, in a statement:
if( isValid && fn() ) ...
the function will never be called isValid is false.
The conditions under which code can break when serviced by an unwary programmer (and these are often less experienced programmers who perform maintenance tasks) are numerous and are probably best avoided.
a source to share
I prefer the former most of the time because it makes the code clearer and easier to read, so it is less error prone. For example, you forgot the parentheses in this second example :) In this case, in fact, I will probably do what you did in the second example, however, if I needed to use this submenu more than a few times, I would go first to make the code more readable. As far as performance goes, I'm sure any sane compiler will be able to optimize this (this is probably why you didn't notice the performance difference).
Also, as the meter pointed out, it also depends on what lock () is doing. In general, if it's a simple getter or something like that, you should be fine.
a source to share
Whatever you choose. For me it depends on how much I will use it; for two lines I could just write this 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 keep looking at it, so use whatever works for you. Of course, if you are in a company with a coding guide, follow it.
a source to share
In this case, I think you should use temporary. Even if you know that the .lock () implementation is inexpensive, this can change. If you don't need to call lock () twice, then don't. The meaning here is that it decouples your code from the lock () implementation. And this is generally good.
a source to share