95

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?

We're looking for long answers that provide some explanation and context. Don't just give a one-line answer; explain why your answer is right, ideally with citations. Answers that don't include explanations may be removed.

  • 95
    Not necessarily. Sometimes it makes the code clearer to use one-time variables, and in most decent programming languages, there's very little (if any) run-time cost for those additional variables. – Robert Harvey Jan 4 '17 at 3:43
  • 75
    It also makes stepping through the code w/ a debugger more difficult. And you also have to be sure (depending on the language) the first expression is not a NULL or an error. – GrandmasterB Jan 4 '17 at 4:39
  • 78
    No, it isn't. In fact, it is good practice to introduce local variables to break chains of method calls. The generated machine code is likely to be identical, and the source code is almost guaranteed to be more readable, hence better. – Kilian Foth Jan 4 '17 at 7:05
  • 48
    Try that. Now plug the debugger and try to see the state of either PowerManager or WakeLock. Realize your mistake. I used to think the same ("what's with all these locals?") until I had to spend most of my time investigating code. – Faerindel Jan 4 '17 at 7:38
  • 42
    Even in your example, your "eliminated" version has a scroll bar and doesn't fit on my screen entirely, which makes it a lot harder to read. – Erik Jan 4 '17 at 9:41

13 Answers 13

235

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 1, 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.

1 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.

  • 2
    You can't know whether there's a lot of code, the optimizer might inline some functions... Otherwise, agreed, striving for readability first is the right move. – Matthieu M. Jan 4 '17 at 9:30
  • 2
    Well, I think the opposite, if I see a local variables getting initialized multiples times on each function where it's used instead of being an attributes when it can be, I will search why, basically I will read thoroughly the lines and compares them just to make sure they're the same. So I will waste time. – Walfrat Jan 4 '17 at 9:46
  • 15
    @Walfrat Local variables getting initialized multiple times? Ouch. Nobody suggested reusing locals :) – Luaan Jan 4 '17 at 10:01
  • 32
    @Walfrat members breed side effects and should only be used when you specifically want to maintain state between calls to public members. This question appears to deal with the use local for temporary storage of intermediate calculations. – Gusdor Jan 4 '17 at 10:38
  • 4
    Q: Should we eliminate local variables if we can? A: No. – simon Jan 6 '17 at 15:42
94

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.

  • 20
    I would add that stacktraces are much less useful when there are many calls on a single line. If you have a null pointer exception on line 50 and there are 10 calls on that line it doesn't narrow things gown much. In a production application, this will often be the bulk of the information you get from a defect report. – JimmyJames Jan 4 '17 at 15:40
  • 3
    Stepping through code is also much harder. If there is call()->call()->call()->call(), it is hard work to step into the third method call. Much easier if there are four local variables – gnasher729 Jan 4 '17 at 19:44
  • 3
    @FrankPuffer - we have to deal with real world. Where debuggers don't do what you propose. – DVK Jan 6 '17 at 0:14
  • 2
    @DVK: Actually there are more debuggers than I thought that let you inspect or watch any expression. MS Visual Studio (since version 2013) has this feature for C++ and C#. Eclipse has it for Java. In another comment, Faerindel mentioned IE11 for JS. – Frank Puffer Jan 6 '17 at 10:12
  • 4
    @DVK: Yeah, many real world debuggers don't do what I propose. But that has nothing to do with the first part of my comment. I just find it crazy to assume that the person who is going to maintain my code will mainly interact with it by means of a debugger. Debuggers are great for certain purposes but if they get the main tool for analyzing code, I'd say that something is seriously wrong and I'd try to fix it instead of making the code more debuggable. – Frank Puffer Jan 6 '17 at 15:30
47

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.

  • 1
    That has as much to do with the styling as the use of variables. The question has now been edited. – Jodrell Jan 6 '17 at 8:51
29

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.

  • I think that has much to do with styling and the use of whitespace as anything. The question has been edited. – Jodrell Jan 6 '17 at 8:53
15

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.

13

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.

  • Regarding your further comments: 1. This shouldn't be a problem if you're following the convention that local variables are declared as closely as possible to where they are used and you're keeping your methods a reasonable length. 2. Not in a programming language with type inference. 3. Well-written code often doesn't need a debugger at all. – Robert Harvey Jan 4 '17 at 15:28
  • 2
    Your first code example only works if you're crafting your objects using fluent interfaces or a "builder," patterns which I would not use everywhere in my code, but only selectively. – Robert Harvey Jan 4 '17 at 15:29
  • 1
    I think the question assumes that the local variables are functionally redundant and therefore that type of interface is required for the case. We could probably achieve removing locals if it were otherwise through various contortions, but I had it in mind we were looking at the simple case. – rghome Jan 4 '17 at 15:35
  • 1
    I find your variant even worse for readability. I may have been exposed to VB.net too much, but my first though when seeing your sample is "Where did the WITH statement go? did I accidentally delete it ?" I need to look twice what is going on and that isn't good when I need to revisit the code many months later. – Tonny Jan 5 '17 at 9:04
  • 1
    @Tonny for me, it is more readable: the locals don't add anything that isn't obvious from the context. I have my Java head on, so no with statements. It would be normal formatting conventions in Java. – rghome Jan 5 '17 at 9:54
6

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.

  • 1
    Great answer. The best code I've written (and have seen from others) often had lots of small (2, 3 lines) methods that could take the place of such temp variables. Related is this controversial take that private methods stink.... Well thought out and I've often found it true. – Stijn de Witt Jan 7 '17 at 22:08
  • The temps in this question already refer to functions, so this answer is irrelevant to OPs question. – user949300 Jan 7 '17 at 22:22
  • @user949300 I don't think it is irrelevant. Yes, the temps in the OP's specific example are the return values from functions or methods. But the OP is very clear that is just an example scenario. The actual question is the much more general "should we eliminate temporary variables when we can?" – Jonathan Hartley Jan 9 '17 at 15:58
  • 1
    OK, I'm sold,** tried to** change my vote. But often when I go beyond the limited scope of the OP question I get dinged. SO can be fickle ... :-). Er, can't change my vote till you edit, whatever... – user949300 Jan 9 '17 at 16:34
  • @user949300 Holy cow! A person who changes their mind upon thoughtful consideration of the issues! You, sir, are a rarity, and I doff my cap to you. – Jonathan Hartley Jan 9 '17 at 17:47
3

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.

2

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.

  • 3
    I'd be interested in how fluid syntax fits into this answer. With correct formatting, fluid syntax is highly readable. – Gusdor Jan 4 '17 at 13:56
  • 12
    That's not what "Law of Demeter" means. The Law of Demeter is not a dot counting exercise; it essentially means "only talk to your immediate friends." – Robert Harvey Jan 4 '17 at 15:32
  • 5
    Accessing the same methods and members via an intermediate variable is still just as severe a violation of the law of Demeter. You have just obscured it, not fixed it. – Jonathan Hartley Jan 5 '17 at 9:13
  • 2
    In terms of the "Law of Demeter", both versions are equally "bad". – Hulk Jan 5 '17 at 9:13
  • @Gusdor agreed, this more a comment on the styling in the question example. The question has now been edited. – Jodrell Jan 6 '17 at 8:58
2

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.

  • To me, even though I have no knowledge of the API, it is very obvious what the code does, even without the variables. getSystemService(POWER_SERVICE) gets the power manager. .newWakeLock gets a wake lock. .acquire acquires it. OK, I don't know all the types, but I can find that out in the IDE if I need to. – rghome Jan 6 '17 at 15:09
  • It may be obvious to you what the code is supposed to do, but you have no clue what it actually does. If I am looking for a bug, all I know is that some code somewhere doesn't do what it is supposed to do, and I have to find which code. – gnasher729 Jan 7 '17 at 13:44
  • @gnasher729 Yes. Bug-hunting was an example I gave of when you would need to carefully read all the details. And one of the things that really helps find the code that isn't doing what it's supposed to do is to be able to easily see what the original author's intent was. – Ben Jan 7 '17 at 21:29
  • when I come back out I've fully understood "one unit" and can then move on to the next statement. Actually no. In fact the local variables prevent that full understanding. Because they are still lingering... taking up mental space... what is going to happen to them in the next 20 statements... drumroll... Without locals you would be sure that the objects were gone and impossible to do anything with them in the next lines. No need to remember about them. I like to return as quickly from methods as possible, for the same reason. – Stijn de Witt Jan 7 '17 at 22:19
2

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.

  • Ehm isn't the revised code doing method chaining? EDIT read the linked article so I see the difference now. Pretty good article thanks! – Stijn de Witt Jan 7 '17 at 22:30
  • Thanks for reviewing it. Anyway, method chainning may work on some cases while just leaving the variable may be the appropriate decision. It is always good to know why people disagree with your answer. – Borjab Jan 8 '17 at 16:31
  • I guess the opposite of "Train Wreck" is "Local Line". It's that train between two major cities that stops at every country village in between just in case someone wants to get on or off, even though most days no-one does. – rghome May 14 at 11:17
1

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())
    {
    }
}
  • This is not really related to local variables but to resource cleanup... I'm not well versed in C# but I would be surprised if using(new SQLConnection()) { /* ... */ } would not be legal too (whether it'd be useful is a different matter :). – Stijn de Witt Jan 7 '17 at 22:29
1

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.

  • 2
    I did not downvote your answer but I would guess that it's related to the fact that local variables are not generally considered 'state' in imperative languages unless you are talking about some sort of long-running method. I agree on the use of final/const as a best-practice but It's pretty unlikely that introducing a variable to simply break up a chained call will lead to issues caused by mutable state. In fact, the use of local variables helps to deal with mutable state. If a method can return different values at different times you can end up with some really interesting bugs otherwise. – JimmyJames Jan 6 '17 at 16:00
  • @JimmyJames: Have added some explanation to my answer. But there is one part of your comment that I didn't understand: "the use of local variables helps to deal with mutable state". Could you explain this? – Frank Puffer Jan 6 '17 at 19:31
  • 1
    For example, let's say I need to check a value and if it's over 10 fire an alert. Suppose this value comes from method getFoo(). If I avoid a local declaration, I will end up with if(getFoo() > 10) alert(getFoo()); But getFoo() might return different values on the two different calls. I could sending an alert with a value that is less than or equal to 10 which is confusing at best and will come back to me as a defect. This kind of thing becomes more important with concurrency. The local assignment is atomic. – JimmyJames Jan 6 '17 at 22:00
  • Very good point. Maybe the reason I don't like these locals.... Are you going to do something with that PowerManager instance after calling this method? Let me check the rest of the method.... mmm ok no. And this WakeLock thing you got... what are you doing with that (scanning rest of method yet again)... mmm again nothing... ok so why are they there? Oh yes supposed to be more readable... but without them I am sure you don't use them any more so I don't have to read the rest of the method. – Stijn de Witt Jan 7 '17 at 22:14
  • At a recent talk, the speaker argued that the best way to show that a variable is final is to write short methods where it is obvious that the variable doesn't change. If final is needed on a local variable in a method, your method is too long. – user949300 Jan 7 '17 at 22:27

protected by gnat Jan 5 '17 at 9:04

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site (the association bonus does not count).

Would you like to answer one of these unanswered questions instead?

Not the answer you're looking for? Browse other questions tagged or ask your own question.