12

Is it good style to use additional, technically superfluous, local variables to describe what's happening?

For example:

bool easyUnderstandableIsTrue = (/* rather cryptic boolean expessions */);

if(easyUnderstandableIsTrue)
{
    // ...
}

When it comes to technical overhead I expect the compiler to optimize this additionaly line away. But is it considered an unnecessary code bloat? In my eyes it reduces the risk of stale comments.

  • 10
    "Is it good style to use additional, technically superfluous, local variables to describe what's happening?". Yes. Not much else can be said here, really. – David Arno Nov 14 '16 at 11:24
  • 3
    I find such a style (i.e. using many intermediate variables with descriptive names) quite helpful when reading my code later. Of course you can write complex expressions and save a few names by not introducing intermediate variables, but why would you want to? Reading code can be quite a challenge even if it is relatively well written, so I don't think needlessly complicating it more is a sensible route. – Mael Nov 14 '16 at 11:30
  • I believe this is called Consolidate Conditional Expression in Martin Fowler's catalog of refactorings. – Brandin Nov 14 '16 at 12:42
16

What's the cost of having an additional variable? In most languages, none, both in compiled and interpreted languages.

What's the benefit of this?

  • Similarly to extracting the cryptic boolean expression to a separate method, you are decreasing the risk of duplicated code, but slightly less than in a case of a separate method. If the conditional expression is reused inside the method itself, you'll be able to reuse the variable; if the expression appears in a different method, you won't.

    Note that unless your programming language allows you to have immutable local variables or you have a way to enforce, style-wise, that none of the variables are reassigned, such refactoring could be risky in the long term. If the value of the variable is changed, it could be very difficult to reason about the code.

  • You are reducing the risk of documentation getting out of sync with the code. Developers tend to update the names of variables and methods more easily than the comments.¹ Thus, it is not unusual to see code such as:

    // Find if the user is an actual author in order to allow her to edit the message.
    if (currentUser.isAdministrator || (message.author == currentUser && !message.locked))

The expression probably started with if (message.author == currentUser), and then evolved to handle the case of locked messages and administrators who don't need to be authors and don't care about locked stuff; however, the comment haven't reflected any of those changes.

Both benefits aren't particularly important, but given the low cost of additional variables, you may indeed consider using them.

Note that if your boolean expression becomes overly complex:²

  • Extract it to a separate method, and:
  • Refactor it into multiple simple boolean expressions.

The example above becomes:

class Message
{
    ...
    public boolean canBeEditedBy(User user)
    {
        ...
        if (user.isAdministrator) {
            return true;
        }

        return this.author == user && !this.locked;
    }
}

...
if (message.canBeEditedBy(currentUser)) // See? Much more readable now!
{
    ...
}

¹ Source: my own observation of my peers developing mostly business software; YMMV. A real research may show different results. Personally, I suppose that when developers read code, they are focusing on the code, and comments are documentation, not code; therefore, they usually don't read comments, so it would be difficult to expect them to update them.

² The overly complex threshold is defined with a simple formula: if half of the developers who review your code express the intent of murder you, the threshold is reached. The boolean expression above is simple enough to require refactoring; however, four parts in a row if ((a && b) || (c && d)) would make it potentially refactorable. Note that if the expression is flat, the number of parts is mostly irrelevant: if (a || b || c || d || ... || z) is readable enough.

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.