Has / can anyone challenge Uncle Bob on his love of removing "useless braces"?

Question

I hate referencing paywalled content, but this video shows exactly what I'm talking about. Precisely 12 minutes in Robert Martin looks at this:

And says "One of my favorite things to do is getting rid of useless braces" as he turns it into this:

A long time ago, in an education far far away, I was taught not to do this because it makes it easy to introduce a bug by adding another indented line thinking it's controlled by the if when it's not.

To be fair to Uncle Bob, he's refactoring a long Java method down to tiny little functions that, I agree, are far more readable. When he's done changing it, (22.18) it looks like this:

I'm wondering if that is supposed to validate removing the braces. I'm already familiar with the best practice. Can Uncle Bob be challenged on this point? Has Uncle Bob defended the idea?

Answer 1

Readability is no small thing.

I'm of a mixed mind when it comes to braces that enclose a single method. I personally remove them for things like single-line return statements, but leaving such braces out did in fact bite us very hard at the last place where I worked. Someone added a line of code to an if statement without also adding the necessary braces, and because it was C, it crashed the system without warning.

I never challenge anyone who is religious about always using braces after that little fiasco.

So I see the benefit of readability, but I am keenly aware of the problems that can arise when you leave those braces out.

I wouldn't bother trying to find a study or someone's published opinion. Everybody has one (an opinion, that is), and because it's a stylistic issue, one opinion is just about as good as any other. Think about the issue yourself, evaluate the pros and cons, and make up your own damned mind. If the shop you work for has a coding standard that covers this issue, just follow that.

Answer 2

You can find several published promotions or rejections of no-brace styles at here or here or wherever bike sheds are painted.

Stepping away from the bike sheds, remember the great OS X/iOS SSL bug of 2014?

if ((err = SSLHashSHA1.update(&hashCtx, &serverRandom)) != 0)
    goto fail;
if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
    goto fail;
    goto fail;
if ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0)
    goto fail;

Yep, "caused" by no-brace blocks https://www.imperialviolet.org/2014/02/22/applebug.html

Preferences may depend on the brace style. If I had to write

if (suiteSetup)
{
    includePage(mode, suiteSetup);
}

I might be inclined to save space too. But

if (suiteSetup) {
    includePage(mode, suiteSetup);
}

only uses one "extra" line.

---

I know you didn't ask, but if I'm working alone, I'm a heretic. If I remove braces, I prefer

if (suiteSetup != null) includePage(mode, suiteSetup);

It doesn't have the same visual problem as the iOS SSL-style bug, but saves even more "unnecessary" lines than Uncle Bob does ;) Reads well if you're used to it, and has mainstream usage in Ruby (includePage(mode, suiteSetup) unless suiteSetup.nil?). Oh well, just know that there are a lot of opinions.

Answer 3

Uncle Bob has many layers of defense against such a mistake that were not as commonplace when "always use braces" was the prevailing wisdom:

1. A strong personal preference for single-line conditionals, so multi-line ones stand out and receive extra scrutiny.
2. An editor that automatically outdents when you insert a second line.
3. A complete suite of unit tests that he runs constantly.
4. A complete suite of integration tests.
5. A peer review done before his code is merged in.
6. A rerun of all the tests by a continuous integration server.
7. Testing done by a product quality department.

I think if someone did publish a challenge to Uncle Bob, he would have a pretty good rebuttal with the above points. This is one of the easiest mistakes to catch early.

Answer 4

For the most-part this is personal preference, however there are some things to consider.

Possible Bugs

While it can be argued that bugs caused by forgetting to add-in braces are rare, from what I've seen that they do happen occasionally (not to forget the famous IOS goto fail bug). So I think this should be a factor when considering your code style (some tools warn about misleading-indentation, so it depends on your tool chain too).

Valid Code (that reads like it might be a bug)

Even assuming your project doesn't suffer from such bugs, when reading code you may see some blocks of code that look like they could be bugs - but aren't, taking some of your mental cycles.

We start with:

if (foo)
    bar();

A developer adds a useful comment.

if (foo)
    // At this point we know foo is valid.
    bar();

Later on a developer expands on it.

if (foo)
    // At this point we know foo is valid.
    // This never fails but is too slow even for debug, so keep disabled.
    // assert(is_valid(foo));
    bar();

Or adds a nested block:

if (foo)
    while (i--) {
        bar(i);
        baz(i);
    }

Or uses a macro:

if (foo)
    SOME_MACRO();

"... Since macros may define multiple lines of code, does the macro use do {...} while (0) for multiple lines? It should because its in our style-guide but I better check just in case!"

---

The examples above are all valid code, however the more content in the code-block, the more you need to read to ensure there aren't any mistakes.

Maybe your code-style defines that multi-line blocks require a brace (no matter what, even if they're not code), but I've seen these kinds of comments being added in production code. When you read it, there is some small doubt that whoever last edited those lines forgot to add a brace, sometimes I feel the need to double-check is working as intended (especially when investigating a bug in this area of the code).

Diff Noise

One practical reason to use braces for single lines is to reduce diff noise.

That is, changing:

if (foo)
    bar();

To:

if (foo) {
    bar();
    baz();
}

... causes the conditional line to show up in a diff as being changed, this adds some small but unnecessary overhead.

• the lines show up as being changed in code-reviews, if your diffing tools are word-based you can easily see that only the brace changed, but that takes more time to check then if the line didn't change at all.
  Having said that, not all tools support word-based diffing, diff (svn, git, hg... etc) will show as if the entire line changed, even with fancy tools, sometimes you may need to quickly look over a plain line-based diff to see what changed.
• annotation tools (such as git blame) will show the line as being changed, making tracking the origin of a line more step to find the real change.

These are both small, and depend on how much time you spend in code-review or tracking-down which commit changed lines of code.

A more tangible inconvenience of having extra lines changes in a diff, theirs higher likely-hood that changes in the code will cause conflicts which merging and need to be manually resolved.

---

There is an exception to this, for code-bases that have { on its own line - it's not a problem.

The diff noise argument doesn't hold if you write in this style:

if (foo)
{
    bar();
    baz();
}

However this isn't such a common convention, so mainly adding to the answer for completeness (not suggesting projects should use this style).

Answer 5

Years ago, I was brought in to debug some C code. The bug was crazy hard to find, but eventually it boiled down to a statement like:

if (debug)
   foo (4);

And it turned out that the person who had written it had defined foo as a macro. A macro with two lines of code in it. And of course, only the first of those two lines was subject to the if. (So the second line was executed unconditionally.)

This may be absolutely unique to C and its preprocessor — which does substitutions before compilation — but I've never forgotten it. That kind of thing leaves a mark on you, and why not play it safe — especially if you use a variety of languages and toolchains and can't be sure such shenanigans aren't possible elsewhere?

Now I indent and use braces differently from everyone else, apparently. For a single line if, I would do:

if (debug) { foo (4); }

so it doesn't take any additional lines to include the braces.

Answer 6

"Uncle Bob" is allowed to have his opinion, you are allowed to have your opinion. No need to challenge him.

If you want an appeal to authority, take Chris Lattner. In Swift, if statements lost their parentheses, but always come with braces. No discussion, it's part of the language. So if "Uncle Bob" starts removing braces, the code stops compiling.

Going through someone else's code and "getting rid of useless braces" is a bad habit. Only causes extra work when the code needs to get reviewed, and when conflicts are unnecessarily created. Maybe "Uncle Bob" is such an incredibly good programmer that he doesn't need code reviews? I wasted one week of my life because one top programmer changed "if (p != NULL)" to "if (! p)" without a code review, hidden in the worst possible place.

This is mostly a harmless style debate. Braces have the advantage that you can insert another line of code without adding braces. Like a logging statement, or a comment (if followed by comment followed by statement is just awful). statement on the same line as if has the practical disadvantage that you have problems with many debuggers. But do whatever you prefer.

Answer 7

My reasons for not removing braces are:

1. reduce decision fatigue. If you always use braces, you never have to decide whether you are going to need braces or not.

2. reduce development drag: even if you strive to eventually extract all multiple lines of logic to methods, having to convert a braceless if to a braced if to add logic is an annoying bit of development drag. So there's the effort of removing the braces, and the effort of adding them again when you need more code. Tiny, but annoying.

Answer 8

A young co-worker has said that the braces which we see as redundant and superfluous are in fact helpful to him. I don't recall exactly why, but if it allows him to write better code quicker, that alone is reason to keep them.

Fwiw, we agreed on a compromise that putting them on one line doesn't make such short preconditions unreadable/distracting to me. E.g.

if (!param) { throw invalid_argument (blahblah); }
if (n < 2) { return 0; }
// real code for function starts here...

I also point out that these introductory preconditions are often control-flow statements for the body (e.g. a return) so the fear of adding a second statement that's meant to be under the same condition and forgetting to write braces is moot. A second statement under the condition would be dead code anyway and not make sense.

I suspect that the fluency in reading issue is caused by the person's brain parser being wired to have the braces as part of the conditional statement. That is not an accurate representation of the grammar of C++, but could be a side-effect of learning certain other languages first, where that is the case.

Answer 9

Having watched that video just now, I noticed that eliminating braces makes it easier to see where code still needs to be refactored. If you need braces, your code can probably be a bit cleaner.

Having said that, when you're in a team, eliminating braces will probably lead to unexpected behavior some day. I say keep them, and you'll save yourself a lot of headaches when things do go wrong.

Answer 10

I was taught not to do this because it makes it easy to introduce a bug by adding another indented line thinking it's controlled by the if when it's not.

If your code is well unit-tested, this kind of "bug" would explode in the face.

Cleaning the code by avoiding any useless and ugly brackets is a practice I follow.