What's the purpose of using braces (i.e. {}) for a single-line if or loop?

StackOverflow https://stackoverflow.com/questions/12193170

Question

I'm reading some lecture notes of my C++ lecturer and he wrote the following:

  1. Use Indentation // OK
  2. Never rely on operator precedence - Always use parentheses // OK
  3. Always use a { } block - even for a single line // not OK, why ???
  4. Const object on left side of comparison // OK
  5. Use unsigned for variables that are >= 0 // nice trick
  6. Set Pointer to NULL after deletion - Double delete protection // not bad

The 3rd technique is not clear to me: what would I gain by placing one line in a { ... }?

For example, take this weird code:

int j = 0;
for (int i = 0 ; i < 100 ; ++i)
{
    if (i % 2 == 0)
    {
        j++;
    }
}

and replace it with: 

int j = 0;
for (int i = 0 ; i < 100 ; ++i)
    if (i % 2 == 0)
        j++;

What's the benefit of using the 1st version?

Was it helpful?

Solution

Let's attempt to also modify i when we increment j:

int j = 0;
for (int i = 0 ; i < 100 ; ++i)
    if (i % 2 == 0)
        j++;
        i++;

Oh no! Coming from Python, this looks ok, but in fact it isn't, as it's equivalent to:

int j = 0;
for (int i = 0 ; i < 100 ; ++i)
    if (i % 2 == 0)
        j++;
i++;

Of course, this is a silly mistake, but one that even an experienced programmer could make.

Another very good reason is pointed out in ta.speot.is's answer.

A third one I can think of is nested if's:

if (cond1)
   if (cond2) 
      doSomething();

Now, assume you now want to doSomethingElse() when cond1 is not met (new feature). So:

if (cond1)
   if (cond2) 
      doSomething();
else
   doSomethingElse();

which is obviously wrong, since the else associates with the inner if.

Edit: Since this is getting some attention, I'll clarify my view. The question I was answering is:

What's the benefit of using the 1st version?

Which I have described. There are some benefits. But, IMO, "always" rules don't always apply. So I don't wholly support

Always use a { } block - even for a single line // not OK, why ???

I'm not saying always use a {} block. If it's a simple enough condition & behavior, don't. If you suspect someone might come in later & change your code to add functionality, do.

OTHER TIPS

It's very easy to accidentally change control-flow with comments if you do not use { and }. For example:

if (condition)
  do_something();
else
  do_something_else();

must_always_do_this();

If you comment out do_something_else() with a single line comment, you'll end up with this:

if (condition)
  do_something();
else
  //do_something_else();

must_always_do_this();

It compiles, but must_always_do_this() isn't always called.

We had this issue in our code base, where someone had gone in to disable some functionality very quickly before release. Fortunately we caught it in code review.

I have my doubts as to the competence of the lecturer. Considering his points:

  1. OK
  2. Would anyone really write (or want to read) (b*b) - ((4*a)*c)? Some precedences are obvious (or should be), and the extra parentheses just add to confusion. (On the other hand, you _should_ use the parentheses in less obvious cases, even if you know that they're not needed.)
  3. Sort of. There are two wide spread conventions for formatting conditionals and loops: 
    if ( cond ) {
    code;
}
    and: 
    if ( cond )
{
    code;
}
    In the first, I'd agree with him. The opening { is not that visible, so it's best to assume it's always there. In the second, however, I (and most of the people I've worked with) have no problem with omitting the braces for a single statement. (Provided, of course, that the indentation is systematic and that you use this style consistently. (And a lot of very good programmers, writing very readable code, omit the braces even when formatting the first way.)
  4. NO. Things like if ( NULL == ptr ) are ugly enough to hinder readability. Write the comparisons intuitively. (Which in many cases results in the constant on the right.) His 4 is bad advice; anything which makes the code unnatural makes it less readable.
  5. NO. Anything but int is reserved for special cases. To experienced C and C++ programmers, the use of unsigned signals bit operators. C++ doesn't have a real cardinal type (or any other effective subrange type); unsigned doesn't work for numeric values, because of the promotion rules. Numerical values on which no arithmetic operations would make sense, like serial numbers, could presumably be unsigned. I'd argue against it, however, because it sends the wrong message: bitwise operations don't make sense either. The basic rule is that integral types are int, _unless_ there is a significant reason for using another type.
  6. NO. Doing this systematically is misleading, and doesn't actually protect against anything. In strict OO code, delete this; is often the most frequent case (and you can't set this to NULL), and otherwise, most delete are in destructors, so you can't access the pointer later anyway. And setting it to NULL doesn't do anything about any other pointers floating around. Setting the pointer systematically to NULL gives a false sense of security, and doesn't really buy you anything.

Look at the code in any of the typical references. Stroustrup violates every rule you've given except for the first, for example.

I'd suggest that you find another lecturer. One who actually knows what he's talking about.

All the other answers defend your lecturer’s rule 3.

Let me say that I agree with you: the rule is redundant and I wouldn’t advise it. It’s true that it theoretically prevents errors if you always add curly brackets. On the other hand, I’ve never encountered this problem in real life: contrary to what other answers imply, I’ve not once forgotten to add the curly brackets once they became necessary. If you use proper indentation, it becomes immediately obvious that you need to add curly brackets once more than one statement is indented.

The answer by “Component 10” actually highlights the only conceivable case where this could really lead to an error. But on the other hand, replacing code via regular expression always warrants enormous care anyway.

Now let’s look at the other side of the medal: is there a disadvantage to always using curly brackets? The other answers simply ignore this point. But there is a disadvantage: it takes up a lot of vertical screen space, and this in turn can make your code unreadable because it means you have to scroll more than necessary.

Consider a function with a lot of guard clauses at the beginning (and yes, the following is bad C++ code but in other languages this would be quite a common situation):

void some_method(obj* a, obj* b)
{
    if (a == nullptr)
    {
        throw null_ptr_error("a");
    }
    if (b == nullptr)
    {
        throw null_ptr_error("b");
    }
    if (a == b)
    {
        throw logic_error("Cannot do method on identical objects");
    }
    if (not a->precondition_met())
    {
        throw logic_error("Precondition for a not met");
    }

    a->do_something_with(b);
}

This is horrible code, and I argue strongly that the following is vastly more readable:

void some_method(obj* a, obj* b)
{
    if (a == nullptr)
        throw null_ptr_error("a");
    if (b == nullptr)
        throw null_ptr_error("b");
    if (a == b)
        throw logic_error("Cannot do method on identical objects");
    if (not a->precondition_met())
        throw logic_error("Precondition for a not met");

    a->do_something_with(b);
}

Similarly, short nested loops benefit from omitting the curly brackets:

matrix operator +(matrix const& a, matrix const& b) {
    matrix c(a.w(), a.h());

    for (auto i = 0; i < a.w(); ++i)
        for (auto j = 0; j < a.h(); ++j)
            c(i, j) = a(i, j) + b(i, j);

    return c;
}

Compare with:

matrix operator +(matrix const& a, matrix const& b) {
    matrix c(a.w(), a.h());

    for (auto i = 0; i < a.w(); ++i)
    {
        for (auto j = 0; j < a.h(); ++j)
        {
            c(i, j) = a(i, j) + b(i, j);
        }
    }

    return c;
}

The first code is concise; the second code is bloated.

And yes, this can be mitigated to some extent by putting the opening brace on the previous line. So: if you insist on curly braces, at least put the opening brace on the previous line.

In short: don’t write unnecessary code which takes up screen space.

In the time since originally writing the answer I’ve mostly accepted the prevailing code style and use braces unless I can put the entire single statement on the previous line. I still maintain that not using redundant braces is usually more readable, and I have still never encountered a bug caused by this.

The codebase I'm working on is scattered with code by people with a pathological aversion to braces, and for the people who come along later, it really can make a difference to maintainability.

The most frequent problematic example I have encountered is this:

if ( really incredibly stupidly massively long statement that exceeds the width of the editor) do_foo;
    this_looks_like_a_then-statement_but_isn't;

So when I come along and wish to add a then-statement, I can easily end up with this if I'm not careful:

if ( really incredibly stupidly massively long statement that exceeds the width of the editor) do_foo;
{
    this_looks_like_a_then-statement_but_isn't;
    i_want_this_to_be_a_then-statement_but_it's_not;
}

Given that it takes ~1second to add braces and can save you at minimum a few confused minutes debugging, why would you ever not go with the reduced-ambiguity option? Seems like false economy to me.

My 2c:

Use Indentation

Obviously

Never rely on operator precedence - Always use parentheses

I wouldn't use words "never and "always", but in general I see this rule being useful. In some languages (Lisp, Smalltalk) this is a non-issue.

Always use a { } block - even for a single line

I never do that and never had a single problem, but I can see how it can be good for students, esp. if they studied Python before.

Const object on left side of comparison

Yoda conditions? No, please. It hurts readability. Just use the maximum warning level when you compile your code.

Use unsigned for variables that are >= 0

OK. Funny enough, I've heard Stroustrup disagree.

Set Pointer to NULL after deletion - Double delete protection

Bad advice! Never have a pointer which points to a deleted or non-existing object.

it is more intuitive and easily understandable. It makes the intent clear.

And it ensures that the code doesn't break when a new user might unknowingly miss the {, } while adding a new code statement.

To add to the very sensible suggestions above, one example I encountered while refactoring some code of where this becomes critical was as follows: I was altering a very large codebase to switch from one API to another. The first API had a call to set Company Id as follows:

setCompIds( const std::string& compId, const std::string& compSubId );

whereas the replacement needed two calls:

setCompId( const std::string& compId );
setCompSubId( const std::string& compSubId );

I set about changing this using regular expressions which was very successful. We also passed the code through astyle, which really made it very much more readable. Then, part way through the review process, I discovered that in some conditional circumstances it was changing this:

if ( condition )
   setCompIds( compId, compSubId );

To this:

if ( condition )
   setCompId( compId );
setCompSubId( compSubId );

which is clearly not what what was required. I had to go back to the beginning do this again by treating the replacement as completely within a block and then manually altering anything that ended up looking goofy (at least it wouldn't be incorrect.)

I notice that astyle now has the option --add-brackets which allows you to add brackets where there are none and I strongly recommend this if you ever find yourself in the same position as I was.

I am using {} everywhere except a few cases where it's obvious. Single line is one of the cases:

if(condition) return; // OK

if(condition) // 
   return;    // and this is not a one-liner

It may hurt you when you add some method before return. Indentation indicates that return is executing when condition is met, but it will return always.

Other example in C# with using statment

using (D d = new D())  // OK
using (C c = new C(d))
{
    c.UseLimitedResource();
}

which is equivalent to 

using (D d = new D())
{
    using (C c = new C(d))
    {
        c.UseLimitedResource();
    }
}

The most pertinent example I can think of:

if(someCondition)
   if(someOtherCondition)
      DoSomething();
else
   DoSomethingElse();

Which if will the else be paired with? Indentation implies that the outer if gets the else, but that's not actually how the compiler will see it; the inner if will get the else, and the outer if doesn't. You would have to know that (or see it behave that way in debugging mode) to figure out by inspection why this code might be failing your expectations. It gets more confusing if you know Python; in that case you know that indentation defines code blocks, so you would expect it to evaluate according to the indentation. C#, however, doesn't give a flying flip about whitespace.

Now, that said, I don't particularly agree with this "always use brackets" rule on its face. It makes code very vertically noisy, reducing the ability to read through it quickly. If the statement is:

if(someCondition)
   DoSomething();

... then it should be written just like this. The statement "always use brackets" sounds like "always surround mathematical operations with parentheses". That would turn the very simple statement a * b + c / d into ((a * b) + (c / d)), introducing the possibility of missing a close-paren (the bane of many a coder), and for what? The order of operations is well-known and well-enforced, so the parentheses are redundant. You'd only use parentheses to enforce a different order of operations than would normally be applied: a * (b+c) / d for instance. Block braces are similar; use them to define what you want to do in cases where it differs from the default, and is not "obvious" (subjective, but usually pretty common-sense).

Because when you have two statements without {}, it's easy to miss an issue. Let's assume that the code looks like this.

int error = 0;
enum hash_type hash = SHA256;
struct hash_value *hash_result = hash_allocate();

if ((err = prepare_hash(hash, &hash_result))) != 0)
    goto fail;
if ((err = hash_update(&hash_result, &client_random)) != 0)
    goto fail;
if ((err = hash_update(&hash_result, &server_random)) != 0)
    goto fail;
if ((err = hash_update(&hash_result, &exchange_params)) != 0)
    goto fail;
    goto fail;
if ((err = hash_finish(hash)) != 0)
    goto fail;

error = do_important_stuff_with(hash);

fail:
hash_free(hash);
return error;

It looks fine. The issue with it is really easy to miss, especially when the function containing the code is way larger. The issue is that goto fail is ran unconditionally. You can easily imagine how frustrating this is (making you ask why last hash_update always fails, after all everything looks fine in hash_update function).

However, that doesn't mean I'm for adding {} everywhere (in my opinion, seeing {} everywhere is annoying). While it can cause issues, it never did for my own projects, as my personal coding style forbids conditionals without {} when they aren't on the same line (yes, I agree that my coding style is unconventional, but I like it, and I use project's code style when contributing to other projects). This makes the following code fine.

if (something) goto fail;

But not the following one.

if (something)
    goto fail;

It makes your code more readable by clearly defining the scope of your loops and conditional blocks. It also saves you from accidental mistakes.

wrt 6: It's safer because deleteing a null pointer is a no-op. So if you happen to accidentally go through that path twice, you won't cause memory corruption be freeing memory that is either free or has been allocated to something else.

This is most of an issue with static file scope objects and singletons that have not very clear lifetimes and have been known to get recreated after they've been destroyed.

In most cases, you can avoid the need for this by using auto_ptrs

I like Luchian's accepted answer, in fact I learned the hard way that he is right, so I do always use braces, even for single-line blocks. However, personally I make an exception when writing a filter, as you are in your example. This:

int j = 0;
for (int i = 0 ; i < 100 ; ++i)
{
    if (i % 2 == 0)
    {
        j++;
    }
}

looks cluttered to me. It separates the for loop and the if statement into separate actions, when really your intent is a single action: to count all of the integers divisible by 2. In a more expressive language, this could be written something like:

j = [1..100].filter(_%2 == 0).Count

In languages which lack closures, the filter cannot be expressed in a single statement, but must be a for loop followed by an if statement. However, it is still one action in the mind of the programmer, and I believe that should be reflected in the code, like so:

int j = 0;
for (int i = 0 ; i < 100 ; ++i)
  if (i % 2 == 0)
{
    j++;
}

One option for helping to prevent the errors that have been described above is to inline what you want to happen when you don't use braces. It makes it much harder to not notice the errors when you try to modify the code.

if (condition) doSomething();
else doSomethingElse();

if (condition) doSomething();
    doSomething2(); // Looks pretty obviously wrong
else // doSomethingElse(); also looks pretty obviously wrong

Looking through the answers no one's explicitly stated the sort of practice I make a habit of, telling the story of your code:

int j = 0;
for (int i = 0 ; i < 100 ; ++i)
{
    if (i % 2 == 0)
    {
        j++;
    }
}

Becomes:

int j = 0;
for (int i = 0 ; i < 100 ; ++i)
{
    if (i % 2 == 0) j++;
}

Putting the j++ on the same line as the if should signal to anyone else, "I only want this block to ever increment j". Of coursethis is only worthwhile if the line is as simplistic as possible, because putting a breakpoint here, as peri mentions, is not going to be very useful.

In fact I've just run across part of the Twitter Storm API that has this 'sort' of code in java, here is the relvant snippet form the execute code, on page 43 of this slideshow:

...
Integer Count = counts.get(word);
if (Count=null) count=0;
count++
...

The for loop block has two things in it, so I wouldn't inline that code. I.e never:

int j = 0;
for (int i = 0 ; i < 100 ; ++i) if (i % 2 == 0) j++;

It's awful and I don't even know if it works (as intended); don't do this. New lines and braces help distinguish separate but related pieces of code, in the same way a comma or a semi-colon do in prose. The above block is as bad a really long sentence with a few clauses and some other statements that never break or pause to distinguish separate parts.

If you really want to telegraph to someone else it's a one-line only job use a ternary operator or ?: form:

for (int i = 0 ; i < 100 ; ++i) (i%2 ? 0 : >0) j++;

But this is verging on code-golf, and I think not great practice (It's not clear to me if I should put the j++ on one side of the : or not). NB I've not run a ternary operator in C++ before, I don't know if this works, but it does exist.

In short:

Imagine how your reader (i.e. the person maintaing the code) interprets your story (code). Make it as clear for them as possible. If you know the novice coder/student is maintaining this, perhaps even leave in as many {} as possible, just so they don't get confused.

If you are a compiler, it doesn't make any difference. Both are the same.

But for programmers, the first one is more clear, easy to read and less error-prone.

Another example of adding curly braces. Once I was searching for a bug and found such code:

void SomeSimpleEventHandler()
{
    SomeStatementAtTheBeginningNumber1;
    if (conditionX) SomeRegularStatement;
    SomeStatementAtTheBeginningNumber2;
    SomeStatementAtTheBeginningNumber3;
    if (!SomeConditionIsMet()) return;
    OtherwiseSomeAdditionalStatement1;
    OtherwiseSomeAdditionalStatement2;
    OtherwiseSomeAdditionalStatement3;
}

If you read the method line-by-line you will notice that there is a condition in the method that returns if it's not true. But actually it looks like 100 other simple event handlers that set some variables based on some conditions. And one day the Fast Coder comes in and adds additional variable setting statement at the end of the method:

{
    ...
    OtherwiseSomeAdditionalStatement3;
    SetAnotherVariableUnconditionnaly;
}

As a result the SetAnotherVariableUnconditionnaly is executed when the SomeConditionIsMet(), but the fast guy didn't notice it because all lines are almost similar in size and even when the return condition is vertically indented it is not-so noticeable.

If the conditional return is formatted like this:

if (!SomeConditionIsMet())
{
    return;
}

it is much noticeable and the Fast Coder will find it at a glance.

I consider the first one to be clear then second. It gives the feeling of closing instructions, with little code is fine when code gets complex {...} helps a lot even if it is endif or begin...end

//first
int j = 0;
for (int i = 0 ; i < 100 ; ++i)
{
    if (i % 2 == 0)
    {
        j++;
    }
}


//second
int j = 0;
for (int i = 0 ; i < 100 ; ++i)
    if (i % 2 == 0)
        j++;
i++;

It is best to set the pointer to NULL when you have finished with it.

Here is an example why:

Class A does the following:

  1. Allocates a block of memory
  2. Then some time later, it delete this block of memory but does not set the pointer to NULL

Class B does the following

  1. Allocates memory (and in this instance it happens to be given the same memory block that was deleted by class A.)

At this point both Class A and Class B have pointers pointing to the same memory block, as far as Class A is concerned this block of memory does not exists because it is finished with it.

Consider the following problem:

What if there was a logic error in Class A which resulted in it writing to memory that now belongs to Class B?

In this particular instance, you will not get an bad access exception error because the memory address is legal, all the while class A is now effectively corrupting class B data.

Class B may eventually crash if it encounters unexpected values and when it does crash, chances are, you will spend quite a long time hunting this bug in class B when the problem is in class A.

If you had set the deleted memory pointer to NULL, you would have gotten an exception error as soon as any logic errors in Class A tried to write to NULL pointer.

If you are worried about the logic error with double delete when pointers are NULL for the second time, then add assert for this.

Also: If you are going to down vote, please explain.

Always having curly braces is very simple and robust rule. However the code may look inelegant when there are lot of braces. If the rules allow to omit curly braces then there should be more detailed style rules and more sophisticated tools. Otherwise it may easily result with chaotic and confusing (not elegant) code. Therefore looking single style rule separate from rest of style guides and tools used is likely fruitless. I will just bring some important details about that rule #3 that haven't even been mentioned in other answers.

First interesting detail is that most proponents of that rule agree to violate it on case of else. In other words they do not demand in review such code:

// pedantic rule #3
if ( command == Eat )
{
    eat();
}
else
{
    if ( command == Sleep )
    {
        sleep();
    }
    else
    {
        if ( command == Drink )
        {
            drink();
        }
        else
        {
            complain_about_unknown_command();
        }
    }
}

Instead, if they see it they may even suggest to write it like that:

// not fully conforming to rule #3
if ( command == Eat )
{
    eat();
}
else if ( command == Sleep )
{
    sleep();
}
else if ( command == Drink )
{
    drink();
}
else
{
   complain_about_unknown_command();
}

That is technically violation of that rule since there are no curly brackets between else and if. Such duality of the rule surfaces when to try to apply it to code base automatically with a mindless tool. Indeed, why to argue, just let a tool to apply style automatically.

Second detail (that is also often forgotten by proponents of that rule) is that the errors that may happen are never only because of violations of that rule #3. Actually those almost always involve violations of rule #1 too (that no one argues with). Again from viewpoint of automatic tools, it is not hard to make a tool that immediately complains when rule #1 is violated and so most of the errors can be caught timely.

Third detail (that is often forgotten by opponents of that rule) is the confusing nature of empty statement that is represented by single semicolon. Most developers with some experience became confused sooner or later by sole misplaced semicolon or by empty statement that is written using sole semicolon. Two curly braces instead of single semicolon are visually way easier to spot.

I have to admit that not always use {} for single line, but it's a good practise.

  • Lets say you write a code without brackets that looks like this:

    for (int i = 0; i < 100; ++i) for (int j = 0; j < 100; ++j) DoSingleStuff();

And after some time you want to add in j loop some other stuff and you just do that by alignment and forget to add brackets.

  • Memory dealocation is faster. Lets say you have big scope and create big arrays inside (without new so they are in stack). Those arrays are removing from memory just after you leave scope. But it is possible that you use that array in one place and it will be in stack for a while and be some kind of rubbish. As a stack have limited and quite small size it is possible to exceed stack size. So in some cases it is better to write {} to prevent from that. NOTE this is not for single line but for such a situations:

    if (...) { //SomeStuff... {//we have no if, while, etc. //SomeOtherStuff } //SomeMoreStuff }

  • Third way to use it is similar with second. It just not to make stack cleaner but to open some functions. If you use mutex in long functions usually it is better to lock and unlock just before accessing data and just after finishing reading/writing that. NOTE this way is using if you have some your own class or struct with constructor and destructor to lock memory.

  • What is more:

    if (...) if (...) SomeStuff(); else SomeOtherStuff(); //goes to the second if, but alligment shows it is on first...

All In All, I cannot say, what is the best way to always use {} for a single line but it is nothing bad to do that.

IMPORTANT EDIT If you write compiling code brackets for a single line does nothing, but if your code will be interpretated it slowes code for very very slightly. Very slightly.

There are a number of possible ways of writing control statements; certain combinations of them may co-exist without impairing legibility, but other combinations will cause trouble. The style

if (condition)
  statement;

will co-exist comfortably with some of the other ways of writing control statements, but not so well with others. If multi-line controlled statements are written as:

if (condition)
{
  statement;
  statement;
}

then it will be visually obvious which if statements control a single line and which ones control multiple lines. If, however, multi-line if statements are written as:

if (condition) {
  statement;
  statement;
}

then the likelihood of someone trying to extend a single-statement if constructs without adding the necessary braces may be much higher.

The single-statement-on-next line if statement may also be problematic if the codebase makes significant use of the form 

if (condition) statement;

My own preference is that having the statement on its own line generally enhances legibility except in cases where there are many if statements with similar control blocks, e.g.

if (x1 > xmax) x1 = xmax;
if (x1 < xmin) x1 = xmin;
if (x2 > xmax) x2 = xmax;
if (x2 < xmin) x2 = xmin;
etc.

in which case I will generally precede and follow such groups of if statements with a blank line to visually separate them from other code. Having a range of statements that all start with if at the same indentation will then provide a clear visual indication that there's something unusual.

Licensed under: CC-BY-SA with attribution
Not affiliated with StackOverflow
scroll top