32

According to the accepted answer on "Rationale to prefer local variables over instance variables?", variables should live in the smallest scope possible.
Simplify the problem into my interpretation, it means we should refactor this kind of code:

public class Main {
    private A a;
    private B b;

    public ABResult getResult() {
        getA();
        getB();
        return ABFactory.mix(a, b);
    }

    private getA() {
      a = SomeFactory.getA();
    }

    private getB() {
      b = SomeFactory.getB();
    }
}

into something like this:

public class Main {
    public ABResult getResult() {
        A a = getA();
        B b = getB();
        return ABFactory.mix(a, b);
    }

    private getA() {
      return SomeFactory.getA();
    }

    private getB() {
      return SomeFactory.getB();
    }
}

but according to the "spirit" of "variables should live in the smallest scope as possible", isn't "never have variables" have smaller scope than "have variables"? So I think the version above should be refactored:

public class Main {
    public ABResult getResult() {
        return ABFactory.mix(getA(), getB());
    }

    private getA() {
      return SomeFactory.getA();
    }

    private getB() {
      return SomeFactory.getB();
    }
}

so that getResult() doesn't have any local variables at all. Is that true?

  • 72
    Creating explicit variables comes with the benefit of having to name them. Introducing a few variables can quickly turn an opaque method into a readable one. – Jared Goguen Mar 12 at 3:12
  • 11
    Honestly, an example in terms of variable names a and b is too contrived to demonstrate the value of local variables. Fortunately, you got good answers anyway. – Doc Brown Mar 12 at 11:27
  • 3
    In your second snippet, your variables aren't really variables. They are effectively local constants, because you do not modify them. Java compiler will treat them same if you defined them final, because they are in local scope and compiler knows what is going to happen with them. It's a matter of style and opinion, if you should actually use final keyword or not. – hyde Mar 12 at 11:28
  • 2
    @JaredGoguen Creating explicit variables comes with the burden of having to name them and the benefit of being able to name them. – Bergi Mar 12 at 15:56
  • 2
    Absolutely zero of code style rules like "variables should live in the smallest scope possible" are universally applicable without thought. As such, you never want to base a code style decision on reasoning of the form in this question, where you take a simple guideline and extend it into a less-obviously-covered area ("variables should have small scopes if possible" -> "variables should not exist if possible"). Either there are good reasons supporting your conclusion specifically, or your conclusion is an inappropriate extension; either way you don't need the original guideline. – Ben Mar 13 at 1:49
110

No. There are several reasons why:

  1. Variables with meaningful names can make code easier to comprehend.
  2. Breaking up complex formulas into smaller steps can make the code easier to read.
  3. Caching.
  4. Holding references to objects so that they can be used more than once.

And so on.

  • 22
    Also worth mentioning: The value is going to be stored in memory regardless, so it actually ends up with the same scope anyway. May as well name it(for the reasons Robert mentions above)! – Maybe_Factor Mar 12 at 5:33
  • 5
    @Maybe_Factor The guideline isn't really there for performance reasons; it's about how easy it is to maintain the code. But that still means that meaningful locals are better than one huge expression, unless you're merely composing well named and designed functions (e.g. there's no reason to do var taxIndex = getTaxIndex();). – Luaan Mar 12 at 8:12
  • 16
    All are important factors. The way I look at it: brevity is a goal; clarity is another; robustness is another; performance is another; and so on. None of these are absolute goals, and they sometimes conflict. So our job is to work out how best to balance those goals. – gidds Mar 12 at 9:53
  • 8
    The compiler will be taking care of 3 & 4. – OrangeDog Mar 12 at 13:16
  • 9
    Number 4 can be important for correctness. If the value of a function call can change (e.g. now(),) removing the variable and calling the method more than once can result in bugs. This can create a situation that is really subtle and hard to debug. It might seem obvious but if you are on a blind refactoring mission to remove variables, it's easy to end up introducing flaws. – JimmyJames Mar 12 at 15:15
16

Agree, variables which are not necessary and does not improve the readability of the code should be avoided. The more variables which are in scope at a given point in code, the more complex that code is to understand.

I don't really see the benefit of the variables a and b in your example, so I would write the version without variables. On the other hand the function is so simple in the first place I don't think it matters much.

It becomes more of an issue the longer the function get and the more variables are in scope.

For example if you have

    a=getA();
    b=getB();
    m = ABFactory.mix(a,b);

At the top of a larger function, you increase the mental burden of understanding the rest of the code by introducing three variables rather than one. You have have to read through the rest of the code to see if a or b are used again. Locals which are in scope longer than they need to are bad for the overall readability.

Of course in the case where a variable is necessary (e.g to store a temporary result) or where a variable does improve the readability of the code, then it should be kept.

  • 2
    Of course, by the point a method gets so long that the number of locals is too large to be easily maintainable, you probably want to break it up anyway. – Luaan Mar 12 at 8:14
  • 4
    On of the benefits of "extraneous" variables such as var result = getResult(...); return result; is that you can put a breakpoint on return and learn what exactly result is. – Joker_vD Mar 12 at 13:24
  • 5
    @Joker_vD: A debugger worthy of its name should be able to let you do all of that anyway (view the results of a function call without storing it in a local variable + setting a breakpoint on function exit). – Christian Hackl Mar 12 at 13:45
  • 2
    @ChristianHackl Very few debuggesr let you do that without setting up possibly complex watches that take time to add in (and if the functions have side effects may break everything). I'll take the with variables version for debugging any day of the week. – Gabe Sechan Mar 12 at 18:08
  • 1
    @ChristianHackl and to build on that further even if the debugger doesn't let you do that by default because the debugger is horribly designed, you can just modify the local code on your machine to store the result and then check it out in the debugger. There's still no reason to store a value in a variable for just that purpose. – The Great Duck Mar 13 at 1:54
11

In addition to the other answers, I'd like to point something else out. The benefit to keeping a variable's scope small isn't just reducing how much code syntactically has access to the variable, but also reducing the number of possible control-flow paths that can potentially modify a variable (either by assigning it a new value or calling a mutating method on the existing object held in the variable).

Class-scoped variables (instance or static) have significantly more possible control-flow paths than locally-scoped variables because they can be mutated by methods, which can be called in any order, any number of times, and often by code outside the class.

Let's take a look at your initial getResult method:

public ABResult getResult() {
    getA();
    getB();
    return ABFactory.mix(this.a, this.b);
}

Now, the names getA and getB may suggest that they will assign to this.a and this.b, we can't know for sure from just looking at getResult. Thus, it's possible that the this.a and this.b values passed into the mix method instead come from the state of the this object from before getResult was invoked, which is impossible to predict since clients control how and when methods are called.

In the revised code with the local a and b variables, it is clear that there is exactly one (exception-free) control-flow from the assignment of each variable to its use, because the variables are declared right before they are used.

Thus, there is a significant benefit to moving (modifyable) variables from class-scope to local-scope (as well as moving (modifyable) variables from the outside of a loop to the inside) in that it simplifies control-flow reasoning.

On the other hand, eliminating variables like in your last example has less of a benefit, because it doesn't really affect control-flow reasoning. You also lose the names given to the values, which doesn't happen when simply moving a variable to an inner scope. This is a trade-off you have to consider, so eliminiating variables might be better in some cases, and worse in others.

If you don't want to lose the variable names, but still want to reduce the scope of the variables (in the case of them being used inside a larger function), you can consider wrapping the variables and their uses in a block statement (or moving them to their own function).

2

This is somewhat language-dependent, but I would say that one of the less obvious benefits of functional programming is that it encourages the programmer and reader of code to not need these. Consider:

(reduce (fn [map string] (assoc map string (inc (map string 0))) 

Or some LINQ:

var query2 = mydb.MyEntity.Select(x => x.SomeProp).AsEnumerable().Where(x => x == "Prop");

Or Node.js:

 return visionFetchLabels(storageUri)
    .then(any(isCat))
    .then(notifySlack(secrets.slackWebhook, `A cat was posted: ${storageUri}`))
    .then(logSuccess, logError)
    .then(callback)

The last is a chain of calling a function on the result of a previous function, without any intermediate variables. Introducing them would make it much less clear.

However, the difference between the first example and the other two is the implied order of operation. This may not be the same as the order it's actually computed, but it's the order in which the reader should think about it. For the second two this is left to right. For the Lisp/Clojure example it's more like right to left. You should be a little wary of writing code that's not in the "default direction" for your language, and "middle out" expressions that mix the two should definitely be avoided.

F#'s pipe operator |> is useful partly because it allows you to write left-to-right things that would otherwise have to be right-to-left.

  • 2
    I've taken to using underscore as my variable name in simple LINQ expressions or lambdas. myCollection.Select(_ => _.SomeProp).Where(_ => _.Size > 4); – Graham Mar 12 at 12:57
  • Not sure this answers OP's question in the slightest... – A C Mar 12 at 19:49
  • 1
    Why the downvotes? Seems like a good answer to me, even if it doesn't directly answer the question it's still useful information – reggaeguitar Mar 12 at 20:06
1

I would say no, because you should read "smallest scope possible" as "among existing scopes or ones that are reasonable to add". Otherwise it would imply that you should create artificial scopes (e.g. gratuitous {} blocks in C-like languages) just to ensure that a variable's scope does not extend beyond the last intended use, and that would generally be frowned upon as obfuscation/clutter unless there's already a good reason for the scope to exist independently.

  • 2
    One problem with scoping in many languages is that it is extremely common to have some variables whose last usage is in the computation of the initial values of other variables. If a language had constructs explicitly for purposes of scoping, that allowed for values computed using inner-scope variables to be used in the initialization of outer-scope ones, such scopes could be be more useful than the strictly-nested scopes many languages require. – supercat Mar 12 at 19:49
1

Consider functions (methods). There it is neither splitting the code in the smallest possible subtask, neither the largest singleton piece of code.

It is a shifting limit, with delimiting logical tasks, into consumable pieces.

The same holds for variables. Pointing out logical data structures, into understandable parts. Or also simply naming (stating) the parameters:

boolean automatic = true;
importFile(file, automatic);

But of course having a declaration at the top, and two hundred lines further the first usage is nowadays accepted as bad style. This is clearly what "variables should live in the smallest scope possible" intends to say. Like a very near "do not reuse variables."

1

Whats somewhat missing as reason for NO is debugging / readabillity. Code should be optimized for that, and clear and concise names help a lot e.g. imagine a 3 way if

if (frobnicate(x) && (get_age(x) > 2000 || calculate_duration(x) < 100 )

this line is short, but already hard to read. Add a few more parameters, and that if spans multiple lines.

can_be_frobnicated = frobnicate(x)
is_short_lived_or_ancient = get_age(x) > 2000 || calculate_duration(x) < 100
if (can_be_frobnicated || is_short_lived_or_ancient )

I find this way easier to read and to communicate meaning - so I have no problem with intermediate variables.

Another example would be languages like R, where the last line is automatically the return value:

some_func <- function(x) {
    compute(x)
}

this is dangerous, is the return expexted or needed? this is clearer:

some_func <- function(x) {
   rv <- compute(x)
   return(rv)
}

As always, this is a judgement call - eliminate intermediary variables if they do not improve reading, otherwise keep or introduce them.

Another point can be debuggability: If the intermediary results are of interest, it can be best to simply introduce an intermediary, like in the R example above. How often this is called for is difficult to imagine and be careful what you check in - too much debugging variables are confusing - again, a judgement call.

0

Referring to just your title: absolutely, if a variable is unnecessary it should be deleted.

But “unnecessary” doesn’t mean that an equivalent program can be written without using the variable, otherwise we’d be told that we should write everything in binary.

The most common kind of unnecessary variable is an unused variable, the smaller the scope of the variable the easier it is to determine that it is unnecessary. Whether an intermediate variable is unnecessary is harder to determine, because it’s not a binary situation, it’s contextual. In fact identical source code in two different methods could produce a different answer by the same user depending upon past experience fixing problems in the surrounding code.

If your example code was exactly as represented, I would suggest getting rid of the two private methods, but would have little to no concern about whether you saved the result of the factory calls to a local variable or just used them as arguments to the mix method.

Code readability trumps everything except working correctly (correctly includes acceptable performance criteria, which is seldom “as fast as possible”).

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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