Should we eliminate local variables if we can?

Question

For example, to keep a CPU on in Android, I can use code like this:

PowerManager powerManager = (PowerManager)getSystemService(POWER_SERVICE);
WakeLock wakeLock = powerManager.newWakeLock(PowerManager.PARTIAL_WAKE_LOCK, "abc");
wakeLock.acquire();

but I think the local variables powerManager and wakeLock can be eliminated:

((PowerManager)getSystemService(POWER_SERVICE))
    .newWakeLock(PowerManager.PARTIAL_WAKE_LOCK, "MyWakelockTag")
    .acquire();

similar scene appears in iOS alert view, eg: from

UIAlertView *alert = [[UIAlertView alloc]
    initWithTitle:@"my title"
    message:@"my message"
    delegate:nil
    cancelButtonTitle:@"ok"
    otherButtonTitles:nil];
[alert show];

-(void)alertView:(UIAlertView *)alertView clickedButtonAtIndex:(NSInteger)buttonIndex{
    [alertView release];
}

to:

[[[UIAlertView alloc]
    initWithTitle:@"my title"
    message:@"my message"
    delegate:nil
    cancelButtonTitle:@"ok"
    otherButtonTitles:nil] show];

-(void)alertView:(UIAlertView *)alertView clickedButtonAtIndex:(NSInteger)buttonIndex{
    [alertView release];
}

Is it a good practice to eliminate a local variable if it is just used once in the scope?

Answer 1

Code is read much more often than it is written, so you should take pity on the poor soul who will have to read the code six months from now (it may be you) and strive for the clearest, easiest to understand code. In my opinion, the first form, with local variables, is much more understandable. I see three actions on three lines, rather than three actions on one line.

And if you think you are optimizing anything by getting rid of local variables, you are not. A modern compiler will put powerManager in a register ¹, whether a local variable is used or not, to call the newWakeLock method. The same is true for wakeLock. So you end up with the same compiled code in either case.

¹ If you had a lot of intervening code between the declaration and the use of the local variable, it might go on the stack, but it is a minor detail.

Answer 2

Some highly upvoted comments stated this, but none of the answers I saw did, so I will add it as an answer. Your main factor in deciding this issue is:

Debuggability

Often, developers spend far more time and effort debugging than writing code.

With local variables, you can:

• Breakpoint on the line where it's assigned (with all the other breakpoint decorations, such as conditional breakpointing, etc...)

• Inspect/watch/print/change-the-value of the local variable

• Catch issues that arise due to casts.

• Have legible stack traces (line XYZ has one operation instead of 10)

Without local variables, all the above tasks are either much harder, extremely hard, or downright impossible, depending on your debugger.

As such, follow the infamous maxim (write the code in a way you would as if the next developer to maintain it after yourself is a crazed lunatic who knows where you live), and err on the side of easier debuggability, meaning use local variables.

Answer 3

Only if it makes the code easier to understand. In your examples, I think it makes it harder to read.

Eliminating the variables in the compiled code is a trivial operation for any respectable compiler. You can inspect the output yourself to verify.

Answer 4

Your question "is it a good practice to eliminate a local variable if it is just used one time in the scope?" is testing the wrong criteria. A local variable's utility does not depend on the number of times it is used but whether it makes the code clearer. Labelling intermediate values with meaningful names can improve clarity in situations such as the one you present since it breaks down the code into smaller, more digestable, chunks.

In my view, the unmodified code you presented is clearer than your modified code and thus should be left unchanged. In fact, I'd consider pulling a local variable out of your modified code in order to improve clarity.

I would not expect any performance impact from the local variables, and even if there is one it is likely to be too small to be worth considering unless the code is in a very tight loop in a speed critical part of the program.

Answer 5

Maybe.

Personally I would hesitate eliminating local variables if a typecast is involved. I find your compact version less readable as the number of brackets start approaching a mental limit that I have. It even introduces a new set of brackets that was not needed in the slightly more verbose version using a local variable.

Syntax highlighting provided by the code editor might mitigate such concerns to some extent.

To my eyes, this is the better compromise:

PowerManager powerManager = (PowerManager)getSystemService(POWER_SERVICE);
powerManager.newWakeLock(PowerManager.PARTIAL_WAKE_LOCK, "abc").acquire();

Thus keeping one local variable and ditching the other one. In C# I would use the var keyword on the first line since the typecast already provides the type information.

Answer 6

Though I accept the validity of the answers favouring local variables, I am going to play devil's advocate and present a contrarian view.

I am personally somewhat against using local variables purely for what amounts to documentation purposes, although that reason itself indicates that the practice does have value: the local variables effectively pseudo-document the code.

In your case, the main problem is the indentation and not the absence of variables. You could (and should) format the code as follows:

((PowerManager)getSystemService(POWER_SERVICE))
        .newWakeLock(PowerManager.PARTIAL_WAKE_LOCK,"MyWakelockTag")
        .acquire();

Compare that with the version with local variables:

PowerManager powerManager=(PowerManager)getSystemService(POWER_SERVICE);
WakeLock wakeLock = powerManager.newWakeLock(PowerManager.PARTIAL_WAKE_LOCK,"abc");
wakeLock.acquire();

With the local variables: the names add some information to the reader and the types are clearly specified, but the actual method calls are less visible and (in my opinion) it does not read as clearly.

Without using local variables: it is more concise and the method calls are more visible, but you might ask for more information about what is being returned. Some IDEs can show you this however.

Three further comments:

1. In my opinion, adding code that has no functional use can sometimes be confusing as the reader has to realise that it has indeed no functional use and is simply there for "documentation". For example, if this method was 100 lines long it would not be obvious that the local variables (and therefore the result of the method call) were not required at some later point unless you read the whole method.

2. Adding the local variables means that you must specify their types, and this introduces a dependency in the code that otherwise would not be there. If the return type of the methods changed trivially (e.g., it was renamed) you would have to update your code, whereas without the local variables you would not.

3. Debugging could be easier with local variables if your debugger doesn't show values returned from methods. But the fix for that is to fix the deficiencies in the debuggers, not change the code.

Answer 7

There's a tension of motivations here. The introduction of temporary variables can make code easier to read. But they can also prevent, and make it harder to see, other possible refactorings, such as Extract Method, and Replace Temp with Query. These latter types of refactorings, when appropriate, often provide much greater benefits than the temp variable would.

On the motivations for these latter refactorings, Fowler writes:

"The problem with temps is that they are temporary and local. Because they can be seen only in the context of the method in which they are used, temps tend to encourage longer methods, because that’s the only way you can reach the temp. By replacing the temp with a query method, any method in the class can get at the information. That helps a lot in coming up with cleaner code for the class."

So, yes, use temps if they make code more readable, especially if that's a local norm for you and your team. But be very aware that doing so can sometimes make it harder to spot bigger alternate improvements. If you can develop your ability to sense when it's worthwhile to go without such temps, and become more comfortable when doing so, then that's probably a good thing.

FWIW I personally avoided reading Fowler's book "Refactoring" for ten years because I imagined there wasn't much to say on such relatively straightforward topics. I was completely wrong. When I eventually read it, it changed my daily coding practices, much for the better.

Answer 8

Well, it is a good idea to eliminate local variables if you can make the code more readable thereby. That long one-liner of yours is not readable, but then it follows a pretty verbose OO style. If you could reduce it to something like

acquireWakeLock(POWER_SERVICE, PARTIAL, "abc");

then I'd say it would probably be a good idea. Perhaps you can introduce helper methods to boil it down to something similar; if code like this appears multiple times then it might well be worthwhile.

Answer 9

Let's consider the Law of Demeter here. In the Wikipedia article on LoD the following is stated:

The Law of Demeter for functions requires that a method m of an object O may only invoke the methods of the following kinds of objects:[2]

1. O itself
2. m's parameters
3. Any objects created/instantiated within m
4. O's direct component objects
5. A global variable, accessible by O, in the scope of m

One of the consequences of following this Law is that you should avoid invoking methods of objects returned by other methods in a long dotted-together string, as is shown in the second example above:

((PowerManager)getSystemService(POWER_SERVICE)).newWakeLock(PowerManager.PARTIAL_WAKE_LOCK,"MyWakelockTag").acquire();

It's truly difficult to figure out what this is doing. To understand it you have to understand what each procedure does, and then decipher which function is being called on which object.The first code block

PowerManager powerManager=(PowerManager)getSystemService(POWER_SERVICE);
WakeLock wakeLock=powerManager.newWakeLock(PowerManager.PARTIAL_WAKE_LOCK,"abc");
wakeLock.acquire();

clearly shows what you're trying to do - you're getting a PowerManager object, obtaining a WakeLock object from the PowerManager, then acquiring the wakeLock. The above follows rule #3 of LoD - you're instantiating these objects within your code, and thus can use them to get done what you need.

Perhaps another way to think about is is to remember that when creating software you should write for clarity, not for brevity. 90% of all software development is maintenance. Never write a piece of code you wouldn't be happy to maintain.

Best of luck.

Answer 10

One thing to note is that code is read frequently, to different "depths". This code:

PowerManager powerManager = (PowerManager)getSystemService(POWER_SERVICE);
WakeLock wakeLock = powerManager.newWakeLock(PowerManager.PARTIAL_WAKE_LOCK, "abc");
wakeLock.acquire();

is easy to "skim". It's 3 statements. First we come up with a PowerManager. Then we come up with a WakeLock. Then we acquire the wakeLock. I can see that really easily just by looking at the start of each line; simple variable assignments are really easy to recognise partially as "Type varName = ..." and just mentally skim over the "...". Similarly the last statement is obviously not the form of the assignment, but it only involves two names, so the "main gist" is immediately apparent. Often that's all I would need to know if I was just trying to answer "what does this code do?" at a highish level.

If I'm chasing a subtle bug that I think is here, then obviously I'll need to go over this in much more detail, and will actually remember the "..."s. But the separate statement structure still helps me do that one statement at a time (especially useful if I need to jump deeper into the implementation of the things being called in each statement; when I come back out I've fully understood "one unit" and can then move on to the next statement).

((PowerManager)getSystemService(POWER_SERVICE))
    .newWakeLock(PowerManager.PARTIAL_WAKE_LOCK, "MyWakelockTag")
    .acquire();

Now it's all one statement. The top level structure is not so easy to skim-read; in the original version of the OP, without linebreaks and indentation to visually communicate any structure, I would have had to count parentheses to decode it into a sequence of 3 steps. If some of the multi-part expressions are nested within each other rather than arranged as a chain of method calls, then it could still come out looking similar to this, so I have to be careful about trusting that without counting parentheses. And if I do trust the indentation and just skim ahead to the last thing as the presumed point of all this, what does .acquire() tell me on its own?

Sometimes though, that can be what you want. If I apply your transformation half-way and wrote:

WakeLock wakeLock =
     ((PowerManeger)getSystemService(POWER_SERVICE))
    .newWakeLock(PowerManager.PARTIAL_WAKE_LOCK, "MyWakeLockTage");
wakeLock.acquire();

Now this communicates to a quick skim "get a WakeLock, then acquire it". Even simpler than the first version. It's immediately obvious that the thing that is acquired is a WakeLock. If obtaining the PowerManager is just a sub-detail that's fairly insignificant to the point of this code, but the wakeLock is important, then it could actually help to bury the PowerManager stuff so it's natural to skim over it if you're just trying to quickly get an idea of what this code does. And not naming it communicates that it is only used once, and sometimes that's what's important (if the rest of the scope is very long, I'd have to read all that to tell whether it's ever used again; though using explicit sub-scopes can be another way to address that, if your language supports them).

Which brings up that it all depends on context and what you want to communicate. As with writing prose in natural language, there are always many ways to write a given piece of code that are basically equivalent in terms of information content. As with writing prose in natural language, how you choose between them should generally not be to apply mechanistic rules like "eliminate any local variable that only occurs once". Rather how you choose to write down your code will emphasise certain things and de-emphasise others. You should strive to make those choices consciously (including the choice to write less readable code for technical reasons at times), based on what you actually want to emphasise. Especially think about what will serve readers who just need to "get the gist" of your code (at various levels), since that will happen far more often than a very close expression-by-expression reading.

Answer 11

This is even an antipattern with its own name: Train Wreck. Several reasons to avoid the replacement have already been stated:

• Harder to read
• Harder to debug (both to watch variables and detect exceptions locations)
• Violation of Law of Demeter (LoD)

Consider if this method knows too much from other objects. Method chaining is an alternative that could help you to reduce coupling.

Also remember that references to objects are really cheap.

Answer 12

The stated question is "Should we eliminate local variables if we can"?

No you should not eliminate just because you can.

You should follow coding guidelines in your shop.

For the most part local variables make the code easier to read and debug.

I like what you did with PowerManager powerManager. For me a lower case of the class means it is just a one time use.

When you should not is if the variable is going to hold onto expensive resources. Many languages have syntax for a local variable that needs to be cleaned up / released. In C# it is using.

using(SQLconnection conn = new SQLconnnection())
{
    using(SQLcommand cmd = SQLconnnection.CreateCommand())
    {
    }
}

Answer 13

One important aspect has not been mentioned in the other answers: Whenever you add a variable, you introduce mutable state. This is usually a bad thing because it makes your code more complex and thus harder to understand and to test. Of course, the smaller the scope of the variable, the smaller the issue.

What you want is actually not a variable whose value can be modified but a temporary constant. Therefore consider to express this in your code if your language allows it. In Java you can use final and in C++ you can use const. In most functional languages, this is the default behaviour.

It is true that local variables are the least harmful type of mutable state. Member variables can cause much more trouble and static variables are even worse. Still I find it important to express as accurately as possible in your code what it is supposed to do. And there is a huge difference between a variable that could be modified later and a mere name for an intermediate result. So if your language allows you to express this difference, just do it.