A step too far

One of the seeds for this blog was some code that I was reviewing. It was based on a recommendation by SonarQube. I don’t have it to hand but it went roughly like this:

    // Prepare query...
    if (auto entry = FetchEntry(query); entry.Active()) {
       // Use entry...
    }

and was based on the rule:

“if”, “switch”, and range-based for loop initializer should be used to reduce scope of variables

The code is perfectly correct and the rule makes sense but I don’t like the result. It seems just as bizarre as seeing code like this:

    foo(); bar();

C++ lets you put as many statements as you like on one like but that doesn’t mean you should do it! Some people out there are probably nodding, some shaking their heads and others shrugging.

I was going to talk about everybody having their own unwritten rules. Instead I can quote another SonarQube rule:

Statements should be on separate lines

Their rule uses exactly the same foo / bar example that I did. It does also list some exceptional circumstances but not the specific one that lead me here.

The “why” is important

Lists of rules in books, online and in automated tools are useful. They have the “what” but often don’t have the “why”. I think automation in particular can end up with good rules used in a bad way.

So lets think about the “why” behind these rules.

“if”, “switch”, and range-based for loop initializer should be used to reduce scope of variables

The advantages:

• The variable doesn’t have to be considered when considering any other code.
• Resources connected to the variable can be freed earlier.
• The identifier can be safely re-used in a different scope.

Statements should be on separate lines

The advantages:

• A single statement is easier to reason about.
• Unlikely to miss any statements.

I already recommend using smaller functions rather than larger ones so the reduction in scope has a smaller impact. Being able to re-use the variable name in a different scope might be handy but not essential. On the other hand, having someone miss code because it’s in a strange place seems like a terrible idea. I want my code to be as quick and easy to understand as possible. For me the first rule is nice but not as fundamental as the second rule.

In the code review I recommend switching to this:

    // Prepare query...
    auto entry = FetchEntry(query);
    if (entry.Active()) {
       // Use entry...
    }

It takes an extra line but, to me, each line is simpler so it’s easier to understand and less likely to have a problem.

I don’t know if you agree or disagree with my preference. I do want other developers to think about the “why” of rules they are using so they can make a considered choice themselves.