Use of Brackets

I recently saw a discussion about the unconditional use of the opening and closing brackets in single line ifs, loops, etc. and I am really impressed at how many strong opinions exist about the matter.

I personally prefer single-line ifs to be written without the brackets, yet some developers say that it is an "unsafe" practice and then they use a bug found in the Apple's SSL code to prove the point:

hashOut.data = hashes + SSL_MD5_DIGEST_LEN;
hashOut.length = SSL_SHA1_DIGEST_LEN;
if ((err = SSLFreeBuffer(&hashCtx)) != 0)
    goto fail;
if ((err = ReadyHash(&SSLHashSHA1, &hashCtx)) != 0)
    goto fail;
if ((err = SSLHashSHA1.update(&hashCtx, &clientRandom)) != 0)
    goto fail;
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;

err = sslRawVerify(...);

Note 1: I will not discuss the use of gotos in this post. The purpose is only to talk about the use or not of the brackets on single-statement ifs;

Note 2: This block of code was got from Naked Security.

If it is not clear, let me bold the error:

hashOut.data = hashes + SSL_MD5_DIGEST_LEN;
hashOut.length = SSL_SHA1_DIGEST_LEN;
if ((err = SSLFreeBuffer(&hashCtx)) != 0)
    goto fail;
if ((err = ReadyHash(&SSLHashSHA1, &hashCtx)) != 0)
    goto fail;
if ((err = SSLHashSHA1.update(&hashCtx, &clientRandom)) != 0)
    goto fail;
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;

err = sslRawVerify(...);

That extra bolded goto fail; is outside the if, being executed unconditionally, yet it is indented like it was part of the if. It appears to be a copy/paste bug and the argument is that if brackets were used all the time, the bug wouldn't happen. Like this:

hashOut.data = hashes + SSL_MD5_DIGEST_LEN;
hashOut.length = SSL_SHA1_DIGEST_LEN;
if ((err = SSLFreeBuffer(&hashCtx)) != 0)
{
    goto fail;
}
if ((err = ReadyHash(&SSLHashSHA1, &hashCtx)) != 0)
{
    goto fail;
}
if ((err = SSLHashSHA1.update(&hashCtx, &clientRandom)) != 0)
{
    goto fail;
}
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;
}

err = sslRawVerify(...);

In this situation, the extra goto fail; is still inside the if and will never be reached. And then, there are questions like: So, why would you prefer the unsafe version of the code?

Well, first, we will still end-up with code that shouldn't be there. In this particular case, it will become unreachable so it would not be an issue but if the code was a file read, a file write, an i++ or many other things we will end-up with the wrong result, so the chances that the brackets will solve the problem are really small. Second, if we believe the bug was caused by a copy/paste issue, I don't think we would have that second version of the code. I believe the code will most probably look like this:

hashOut.data = hashes + SSL_MD5_DIGEST_LEN;
hashOut.length = SSL_SHA1_DIGEST_LEN;
if ((err = SSLFreeBuffer(&hashCtx)) != 0)
{
    goto fail;
}
if ((err = ReadyHash(&SSLHashSHA1, &hashCtx)) != 0)
{
    goto fail;
}
if ((err = SSLHashSHA1.update(&hashCtx, &clientRandom)) != 0)
{
    goto fail;
}
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;
}

err = sslRawVerify(...);

Now we have an entire extra block with the goto fail;, being a valid block that will be executed unconditionally. It is the same problem found in the first block of code and it can be caused by the exact same copy/paste situation. Yet, this code is bigger and considering the real unit has many, many similar conditions, we will have more pages to take a look at. To me, it seems more error prone than the original code as those extra brackets are only polluting the code, making it hard to spot anything.

Returning to my preference of no-brackets on single line ifs, it doesn't mean I would write code like the first presented one. I always put an extra empty line when the if begins and ends, isolating each if and avoiding that strange "zig-zag" like indentation that makes things harder to read. That is, I would write this:

hashOut.data = hashes + SSL_MD5_DIGEST_LEN;
hashOut.length = SSL_SHA1_DIGEST_LEN;

if ((err = SSLFreeBuffer(&hashCtx)) != 0)
    goto fail;

if ((err = ReadyHash(&SSLHashSHA1, &hashCtx)) != 0)
    goto fail;

if ((err = SSLHashSHA1.update(&hashCtx, &clientRandom)) != 0)
    goto fail;

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;

err = sslRawVerify(...);

And as I am really used to see "small blocks" as two lines only, when I see 3 lines together without brackets, it simply feels wrong. I don't need to understand the code, I take a fast look, without really reading anything and I spot something odd. Then I will naturally take a better look at it.

I am not saying that everybody must write code like I do but, honestly, I can't agree that using brackets all the time will make things "safer". Copy/paste errors can happen as easily as they would happen without the brackets and extra invalid calls can still be a problem even when they are grouped inside the if, so it becomes more a matter of preference than anything else.

But, before someone makes a complaint, I only think this works when we use the brackets on their own lines. When brackets are put in the same line as the if, I agree that using the brackets all the time makes things safer. Let's see an example:

if ((someFlagsEnum & SomeFlagsEnum.Flag0) == SomeFlagsEnum.Flag0){
  SomeCode();

  if ((someFlagsEnum & SomeFlagsEnum.Flag1) == SomeFlagsEnum.Flag1)
    DoThis();
    DoThat();
  }

  if((someFlagsEnum & SomeFlagsEnum.Flag2) == SomeFlagsEnum.Flag2){
    SingleLineHere();
}

This contrived example has two opening-brackets and two closing-brackets, yet one of the opening-brackets is at the wrong place. Different from the previous cases, the opening-brackets aren't visually close to the if itself, so we may not look to the right at all to spot them. So, forcing all ifs to have the opening bracket will become safer, as 3 ifs will imply 3 closing-brackets. In this case, we will not try to match an opening-bracket with a closing-bracket, we will try to match ifs or any other "block-accepting" instructions to close-brackets.

What I see is that, in the end, we always match things that are horizontally aligned, be it an if with its closing-bracket (so we see the if as the opening and, considering how the language works, the opening-bracket becomes mandatory) be it an opening-bracket with its closing-bracket (in this case, the opening-bracket must be in its own line... maybe with the option to make both the opening and closing brackets disappear if we don't need to group 2 or more instructions). What we can't do is mix the two standards and when something wrong happens, say that the fault is because of the single-line ifs. Code that doesn't follow a standard is always a problem.

My Personal Rules

So, to explain my own rules, I allow single-line ifs to exist without brackets, and they can even be composed of other single line ifs, like:

if (someCondition)
  if(someOtherCondition)
    DoTheCall();

Yet, a block will be needed on the outer if when the inner if has an else. So, this is invalid:

if (someCondition)
  if (someOtherCondition)
    DoTheCall();
  else
    ElseCall(); // Is this else really from the second if?
                // Or is it working over the first one?

And it should be written like this:

if (someCondition)
{
  if (someOtherCondition)
    DoTheCall();
  else
    ElseCall();
}

Ifs that have multiple lines of code inside and have an else will use the brackets on the else, even when the else has only one call. Like this:

if (someCondition)
{
  CallA();
  CallB();
}
else
{
  SingleCallC();
}

Yet the opposite is allowed. You can have a single line if without brackets followed by a multiple-line else:

if (someCondition)
  CallA();
else
{
  CallB();
  CallC();
}

I also indent multiple using clauses with the same standard as I indent ifs. I really don't know why Visual Studio indents multiple usings like this:

using(var a = new DisposableObject1())
using(var b = new DisposableObject2())
using(var c = new DisposableObject3())
  CodeThatUses(a, b, c);

To me, they should be indented like this:

using(var a = new DisposableObject1())
  using(var b = new DisposableObject2())
    using(var c = new DisposableObject3())
      CodeThatUses(a, b, c);

Finally, to keep the standard, I never write single-line ifs, usings, etc. with brackets. If a block of code is changed to become a one-line code, I really lose time removing the extra brackets that became unnecessary.

Again, I am not saying that you should use my standard. It works really fine for me, making any mistakes easier to spot simply because I am used to how things should look and any deviation immediately looks odd. So, in my opinion, any standard that makes wrong code look odd and is easy to read in general is a valid standard. The problem is that even "easy to read" is affected to how adapted we are to a standard. That is, it is possible to have real bad standards (or no standards at all) and still consider the code easy to read because we got used to it. It is also possible to have good standards and consider them bad because we aren't adapted to them. It is really a matter of taste (but we can probably measure how good a standard is by seeing how easy most people adapt to them... and we will see that reading code in a standard and writing it in the same standard have different bars). In the end, we can have our own preferences but it is better to follow other standards when working on projects that already have their own standard. But, definitely, the use of brackets is not directly related to the "safety" of the code and using them all the time is not a guarantee that code will not have bugs caused by copy/paste or any other simple distraction.

Warnings

As a final comment, considering the first block of code, I believe any decent compiler will generate a warning saying that there's code that will never be reached. So, if the compiler is generating a warning and developers are ignoring it (or disabling it), I believe there's a much bigger issue than the use of brackets.

CodeProject