328

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?

user2864740
  • 54,112
  • 10
  • 112
  • 187
JAN
  • 18,509
  • 49
  • 147
  • 268
  • 260
    Readability and maintainablity. It's not immediately obvious what statement block 'j++' belongs to, and that adding code after it won't be associated with the if statement. – The Forest And The Trees Aug 30 '12 at 08:52
  • 27
    I was always told to use the curly braces {} for these lines for a couple of reasons. It makes the code clearer to read. Also someone else in six months time may need to edit your code so clarity is important and with the braces there an error is less likely to happen. There's nothing techincally more correct about it, it's more just a matter of good practice. Keep in mind a project may have thousands and thousands of lines of code for some new guy to plought through! – RossC Aug 30 '12 at 08:53
  • 4
    I do it because code changes, and I get annoyed when I have to add braces in order to add a line of code. "Always" is a strong word, though. – Mike Seymour Aug 30 '12 at 08:55
  • 5
    I don't think that `5. Use unsigned for variables that are >= 0` is a nice trick. If you're decrementing an unsigned int == 0, you'll get an underflow. Which can easily occur. – Pascal Lécuyot Aug 30 '12 at 09:00
  • 30
    I don't agree with 6, as it will hide a double deletion and potentially hide logic errors. – Luchian Grigore Aug 30 '12 at 09:08
  • 12
    I wouldn't bother with 4. Strange to read the code it makes, and it only catches a small class of error, which any compiler should warn about anyway. – Mike Seymour Aug 30 '12 at 09:08
  • 28
    #5 might be tricky - consider this loop: `for (unsigned i = 100; i >= 0; --i)`. – Archie Aug 30 '12 at 09:27
  • 37
    Btw, `(i % 2 == 0)` contradicts (2). You are relying on operator precedence, and the meaning is of course `((i % 2) == 0)` rather than `(i % (2 == 0))`. I would classify rule 2 as "a valid sentiment but 'always' is wrong". – Steve Jessop Aug 30 '12 at 10:04
  • 8
    @Archie: in that loop, `i` is not "a variable that is >= 0`. It's >=0 *except* when it's expected to go negative to terminate the loop. So it should not be unsigned, and rule 5 does not say that it should be unsigned :-) – Steve Jessop Aug 30 '12 at 10:08
  • 1
    @Federico It is basically to avoid double delete. This approach has its pros and cons. This post should give you more details: [Is it good practice to NULL a pointer after deleting it?](http://stackoverflow.com/a/1931171/656598) – oyenamit Aug 30 '12 at 10:19
  • 6
    @TheForestAndtheTrees: "It's not immediately obvious what statement block 'j++' belongs to..." Tell that to Guido van Rossum. – P Daddy Aug 30 '12 at 11:53
  • 18
    "Never rely on operator precedence" implies that, at the very least, the expression `i % 2 == 0` needs parentheses: `(i % 2) == 0`. And, of course, if you do something like `i = 2 * j + 1;` you need multiple parentheses: `i = ((2 * j) + 1);`. Your code would end up looking like LISP (Lots of Infernal Stupid Parentheses. Not that that's what you asked about, but stupid rules lead to stupid code. Some of these guidelines are coding solutions for beginners errors. Most programmers don't stay beginners for long, and following beginners' rules is not a good idea. Learn to write correct code. – Pete Becker Aug 30 '12 at 12:18
  • 1
    As you have not said otherwise, one must assume we are talking about the current version of C++ and as such you should not use raw pointers with out good reason, and if you are, when setting to a 'non valid state' set it to `null_ptr` – thecoshman Aug 30 '12 at 12:25
  • 7
    You should **never use `delete`** in modern C++. Always use smart pointers instead. – Neil G Aug 30 '12 at 14:09
  • 3
    @ron: `std::can.open()` :) – Component 10 Aug 30 '12 at 14:09
  • 9
    The most popular reason seems to be "you can forget to add braces when you later decide to add more statements to the loop body". However, I think this is an exaggeration. Virtually every IDE I've used automatically corrects indentation anyway and such things would stand out like a sore thumb. – Superbest Aug 30 '12 at 14:59
  • 6
    I find it remarkable for a question with so many answers and comments (including those on answers) that no one has noted: the primary audience of code is other programmers and yourself. Possibly many years in the future with little knowledge of this system and the assumptions you are applying. Always assume that someone reading your code is against major deadlines (production is down *now* and costing $millions per hour), they are psychotic, *and* know where you live. – Richard Aug 30 '12 at 15:19
  • 4
    @Richard: unfortunately, under those conditions you cannot win. When given a choice between two rules, there is one hypothetical psychotic who is unable to read one and another hypothetical psychotic who is unable to read the other. So whichever one you code to, somebody finds your code unreadable. Where by "finds unreadable" I mean, "is willing to argue against the readability in the context of ideal style", not necessarily that they actually can't read it. – Steve Jessop Aug 30 '12 at 15:40
  • 1
    It's so your code also works in Perl without change. – Peter Eisentraut Aug 30 '12 at 15:42
  • 2
    You say this is C++? Objects automagically call destructors when they exit their scope... so putting local vars in their blocks frees them at the end of their block, which is a decent thing to have in C++. – Shark Aug 30 '12 at 16:01
  • 8
    I can't get behind number 6. I had a bug where I blanked and forgot that the pointer I was deleting was a shared resource. If I had set it to NULL after deleting it, my program would've continued to run, spitting out wrong answers. Because I didn't set it to null, it segfaulted the next time I ran it, letting me find the actual problem. Never allow the computer to sweep your logic errors under the rug and forget about them. – pR0Ps Aug 30 '12 at 16:51
  • @SteveJessop More a case hypothetical psychotic put into a highly stressed position because the statement wasn't in a block and was missed in test because that edge case wasn't considered. I've been burnt (but not badly) by this. (If someone—however psychotic—cannot read consistently formatted code whatever one of the common styles then the root problem is their lack of ability). TL/DR: not about placement of braces but their presence, and I agree with rule #3 for those exceptional cases it saves my arse. – Richard Aug 30 '12 at 17:42
  • @Richard: fair enough, so they'll come after me with an axe if they make an error modifying my code, but they won't come after me with an axe if they consider my braces to be obstructing their view of my code ;-) As it happens, I do in practice put the braces in unless I'm writing `if (condition) statement;` on a single line, which is rare. So I should be mostly safe from your kind of psychotic, but not entirely. Unfortunately I put the opening brace on the same line, so RMS will get me anyway. – Steve Jessop Aug 30 '12 at 17:50
  • 2
    I think I only agree with the first item in that list. You should be confident in the correctness of your code because you tested it, not because you followed some set of rules in the hopes of avoiding typos. – Winston Ewert Aug 30 '12 at 18:30
  • 1
    @Luchian Grigore, I strongly agree with you on that. Double deletion is usually a logical error. Unless a pointer by design can be null, one should not set it to null. – cleong Aug 30 '12 at 19:51
  • 11
    #1 is the only guideline here that isn't questionable – Joren Aug 30 '12 at 20:14
  • 5
    In your examples, your loops should be `for (int i = 0 ; 100 > i ; ++i)` (per Rule 4)... – TMN Aug 30 '12 at 20:20
  • 1
    @TomTanner The question "solicits debate, arguments [...] or extended discussion". The answers below are more than proof of that. But there are already 4 re-open votes, so rest assured the debate, arguments and extended discussion will soon come back to life. – jogojapan Aug 31 '12 at 12:18
  • There's only [OTBS](https://en.wikipedia.org/wiki/Otbs#Variant:_1TBS). – DanMan Sep 01 '12 at 15:34
  • 3
    My general rule with parenthesis is, if I ever have to ask myself what the operator precedence is for an expression, use parens to make it explicit. In particular, I prefer to use parens to group expressions that use multiple boolean operators. I don't think anyone should have to read the operator precedence documentation to read my code. But if you don't know how `x + 5 * y == z` groups, maybe you should take a course in algebra before doing any programming. – Nate C-K Sep 02 '12 at 02:35
  • @NateC-K That does depend on the language, though. I know of at least one that _doesn't_ have operator precedence, so your example would indeed get parsed as `((x + 5) * y) == z` – Izkata Sep 02 '12 at 04:46
  • 1
    _"Const object on left side of comparison // OK"_. You mean doing a comparison like so: `if (5 == myVar)`. [I like Yoda](http://www.codinghorror.com/blog/2012/07/new-programming-jargon.html), but not *that* much. – Laoujin Sep 02 '12 at 15:46
  • @Izkata: If you're programming in MUMPS or another such language, obviously the rules are different. But then using parens becomes a matter of necessity, not style, so it's not really relevant to this conversation. – Nate C-K Sep 03 '12 at 20:18
  • What if your Single-line is a `macro` ... Hum ! – Emadpres Sep 05 '12 at 06:16
  • 1
    @EmAdpres: if it won't work, you should correct the macro (a macro should be syntactically either an expression or a single statement), rather than adding braces all over your code. – Yakov Galka Oct 27 '12 at 14:53
  • "What's the benefit"? because otherwise you put yourself at risk of fatal idiocy like this: http://www.dwheeler.com/essays/apple-goto-fail.html ...or your penchant for careless ugly style leading to mishaps - and afterwards pointless inconsistency - like this: https://lkml.org/lkml/2015/12/14/461 also @Emadpres, braces can be used in single-line contexts, plus you can artificially line-break macros using `\\` _anyway_ – underscore_d Apr 21 '16 at 09:07
  • "Const object on left side of comparison // OK" - I used to think so. This is clever, I thought! I'm being clever, I thought! I'm avoiding accidental assignment, I thought! Yeah - avoiding accidental assignment - _while_ inverting the comparison needed, not always getting that right in a rush, and so sometimes inverting the _result_ and response. It's not actually clever, not worth the risk, and baffles most who see it - often including _yourself_ - probably because it contradicts intuition and math lessons. C/++ made a questionable decision on `=` vs `==` - but not bad enough to justify this. – underscore_d Apr 21 '16 at 09:17

23 Answers23

515

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.

Community
  • 1
  • 1
Luchian Grigore
  • 236,802
  • 53
  • 428
  • 594
  • Nice, but just two points. 1) Even worse is having j++;i++; on same line thinking it is okay, its not. 2) A good IDE should tell you that i is unidentified since it is out of scope. – Science_Fiction Aug 30 '12 at 08:55
  • 5
    @Science_Fiction: True, but if you add `i++` *before* `j++`, then both variables will still be in scope when they're used. – Mike Seymour Aug 30 '12 at 09:03
  • 26
    This sounds very reasonable, but neglects the fact that the editor does the indentation, not you, and it will indent the `i++;` in a way that shows immediately that it is not part of the loop. (In the past, this might have been a reasonable argument, and I've seen such problems. About 20 years ago. Not since.) – James Kanze Aug 30 '12 at 09:53
  • 43
    @James: that's not a "fact", though, it's your workflow. And the workflow of a lot of people, but not of everyone. I don't think it's *necessarily* an error to treat C++ source as a plain text file, rather than the output of a WYSIWYG editor (vi/emacs/Visual Studio) that enforces formatting rules. So this rule is editor-agnostic beyond what you need, but not beyond what people actually use to edit C++. Hence "defensive". – Steve Jessop Aug 30 '12 at 10:11
  • 31
    @JamesKanze Are you really relying on the assumption that everyone always works in powerful IDEs? The last C I wrote was in Nano. Even given that, one of the first things I tend to turn off in an IDE is the auto-indentation - because the IDE tends to get in the way of my _non-linear_ workflow, trying to correct my 'mistakes' based on incomplete information. IDEs aren't very good at auto-indenting every programmer's natural flow. Those programmers who use such features tend to merge their style to their IDE, which is fine if you only use one IDE but notsomuch if you work across many. – Rushyo Aug 30 '12 at 11:17
  • 2
    @JamesKanze: I'd bet that a lot of programmers would write those two instructions in a single line, since they are so short. And then kaboom. – Gorpik Aug 30 '12 at 12:10
  • Your examples showing the reality of what is being done should use the braces so that it is 100% clear how the compiler will treat the short hand examples – thecoshman Aug 30 '12 at 12:23
  • 10
    “this is a silly mistake, but one that even an experienced programmer could make.” – Like I said in my answer, I don’t believe that. I think it’s an entirely contrived case which doesn’t pose a problem in reality. – Konrad Rudolph Aug 30 '12 at 13:27
  • 4
    @KonradRudolph you've been lucky enough to not have to fix bugs because of this. I have. And in some cases, it's not at all obvious. – Luchian Grigore Aug 30 '12 at 13:31
  • 1
    @SteveJessop A C++ source is "plain text", and I regularly treat it as such, using `grep` or `diff` on it, for example. But developing high quality software is difficult, and it would be foolish not to use all of the tools available to me. Source code editors abound; why use an inappropriate tool, when appropriate ones are readily available? (And the editors I've seen---the three you mention---don't enforce formatting rules. You can always override them. But when the implicit formatting doesn't conform to what you expect, it's generally a sign that you've made a mistake.) – James Kanze Aug 30 '12 at 13:41
  • 1
    @Gorpik I've never seen a coding guideline which would allow a programmer to put two statements in a single line. – James Kanze Aug 30 '12 at 13:42
  • @James: OK, "enforce" maybe was the wrong word, and of course everything a decent IDE does to the source is optional and configurable. Indecent IDEs sometimes have their own ideas. But to the extent that they don't enforce it, it's incorrect to say that the indenting is done by the editor rather than the programmer. My preferred degree of "auto-indenting" by my editor is to maintain the current indentation level when I hit the return key, and I only change from that if I want the editor to produce a particular style that's not my own (i.e. when coding to a strict standard). – Steve Jessop Aug 30 '12 at 14:05
  • And following up on the "coming from Python"... You also need a rule to put a semicolon after each (non-compound) statement. I find I forget these more often than I forget unnecessary braces. – James Kanze Aug 30 '12 at 14:22
  • @SteveJessop Interesting. I've yet to find an editor which will indent entirely to my satisfaction, so I end up "correcting" the editor's indentation from time to time. But having the editor do the first indentation automatically, depending on braces, parentheses, etc., has turned up so many errors in my typing that I wouldn't dream of working without it. Sure, the compiler will complain later, but the earlier I find out about the error, the better. – James Kanze Aug 30 '12 at 14:39
  • When I've used editor-driven indenting for real code, IIRC it's been for Java. It was a while ago, but I don't remember it catching errors, I did it because Sun's indentation rules are (or at least were, maybe Oracle changed them) so stupid that I have trouble following them manually. I certainly didn't feel the lack of it when I wasn't using it. One thing I do find, though, is that whatever errors my toolchain catches, I make them more often than errors it doesn't. – Steve Jessop Aug 30 '12 at 15:05
  • What about non-C languages? Delphi has scope of variables within a procedure, not further localized. The order in which i++ (or Inc(i)) or j++ is called does not extend the loop. – R-D Aug 30 '12 at 15:20
  • 3
    How these two top answers with their mostly preposterous offerings got such a praise by votes (and how the question became so popular) is entirely beyond me; I only have to assume that C# and Java folk simply overran this `c++` question. – eq- Sep 01 '12 at 03:21
  • The 3rd reason regarding nested if's occurs a LOT when fixing errors in legacy C/C++ code (think K&R style from the days before robust IDE's). Trying to figure out if the original coder meant the code to be executed as written, or just messed up is problematic when those coders are long retired (or dead). In these cases where the language has demonstrated an ability to cause confusion, make your intent explicit. Future generations will thank you. – mtdarland Sep 04 '12 at 19:10
  • 1
    I've made a lot of really stupid mistakes, but none of them have been caused by an unbracketed, one line if. This is one of the few "best" practices I just don't agree with. (This comment will bite me tomorrow at work, just wait) – Chad Schouggins Sep 04 '12 at 23:48
  • @ChadSchouggins you're among the lucky ones. I use `{}` only when I predict something wrong *might happen* (ergo the term defensive). It doesn't cost me anything to surround a statement with `{}`, but it *can* save you trouble in the future, so why not? – Luchian Grigore Sep 05 '12 at 06:07
  • That's very nice explanation using if else example. Thanks Luchian Grigore this is really cool. – Ravia Sep 05 '12 at 10:05
  • "If you suspect someone might come in later & change your code to add functionality, do." - Am I the only one that thinks ALL of my code is subject to someone (myself included) coming in 6 months down the road and changing functionality? The bottom line is that it makes the code more maintainable and readable, which should be a goal of all code. – sdm350 Sep 05 '12 at 15:15
  • I always use { }, even in the trivial case, because when is required to add another statement in the same block, this detail could be accidentally forgotten. Believe me, I have seen this too many times to say this. – Guillermo Gutiérrez Sep 08 '12 at 05:50
  • When I read the first example, I thought the `i++` would fall through to the `for` loop (and would be incremented on every iteration). After seeing the equivalent code, I realised I didn't see that some braces were missing. So, I definitely think an experienced programmer could make the same mistake. My personal rule is to *never* have an inline block within an inline block, but I generally avoid inline blocks altogether, unless they improve readability. – mgthomas99 Jul 02 '18 at 09:10
330

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.

BlueRaja - Danny Pflughoeft
  • 75,675
  • 28
  • 177
  • 259
ta.speot.is
  • 25,998
  • 8
  • 62
  • 93
  • 3
    Ohh boy!! it's defined behavior that `must_always_do_this();` will execute if you comment //do_something_else(); – Mr.Anubis Aug 30 '12 at 09:09
  • Good answer, but seems like the first sentence got it the wrong way around so I fixed it. Just saying in case I'm wrong because it's odd that no one else noticed and changed it. – Supr Aug 30 '12 at 13:02
  • 1
    @Supr, as it was first written, he's saying it's difficult to break the correct flow if you use braces, and then gives an example of how easy it is to break without having the code properly bracketed – SeanC Aug 30 '12 at 13:17
  • @SeanCheshire Ahh, now I see what was meant. I interpreted "change" as "disabling statement on purpose". My bad. Nice, BlueRaja. – Supr Aug 30 '12 at 13:54
  • 21
    I ran into this just the other day. `if(debug) \n //print(info);`. Basically took out a whole library. – Kevin Sep 01 '12 at 04:37
  • 4
    `Fortunately we caught it in code review.` Ouch! That sounds so wrong. `Fortunately we caught it in unit tests.` would be much better! – BЈовић Sep 03 '12 at 12:39
  • 4
    @BЈовић But what if the code was in a unit test? The mind boggles. (Just kidding, it's a legacy app. There are no unit tests.) – ta.speot.is Sep 04 '12 at 00:25
59

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.

ЯegDwight
  • 23,615
  • 10
  • 43
  • 51
James Kanze
  • 142,482
  • 15
  • 169
  • 310
  • 14
    The number 4 might be ugly however there is a purpose to it. It is trying to prevent if (ptr = NULL). I don't think I have ever used `delete this`, is it more common than I have seen? I don't tend to think setting a pointer to NULL after use is that bad of a thing to do but YMMV. Maybe it's just me but most of his guidelines don't seem that bad. – Firedragon Aug 30 '12 at 10:47
  • 17
    @Firedragon: Most compilers will warn about `if (ptr = NULL)` unless you write it as `if ((ptr = NULL))`. Have to agree with James Kanze that the ugliness of having `NULL` first makes it a definite NO for me. – Leo Aug 30 '12 at 11:50
  • 34
    @JamesKanze: I'd have to say that I disagree with most of what you've stated here - although I appreciate and respect your arguments for arriving at them. *To experienced C and C++ programmers, the use of unsigned signals bit operators.* - I don't agree at all: The use of *bit operators* signals the use of bit operators. To me, the use of `unsigned` indicates an *aspiration* on the part of the programmer that the variable should represent only positive numbers. Mixing with signed numbers will usually cause a compiler warning which was probably what the lecturer was intending. – Component 10 Aug 30 '12 at 11:53
  • 7
    I disagree with many of your points (especially 3 & 5) but +1 anyway since you show that the dogmatic list of the lecturer is a mistake no matter what. Such rules, especially when given without justification, are just bad. – Konrad Rudolph Aug 30 '12 at 12:01
  • 18
    *To experienced C and C++ programmers, the use of unsigned signals bit operators* Or not. `size_t`, anyone? – ta.speot.is Aug 30 '12 at 12:15
  • 1
    @ta.speot.is: `size_t` is an unsigned type of course, but it isn't necessarily the same type as `unsigned`, and using it doesn't use the keyword `unsigned`. IMO you can by all means use `size_t` for sizes, certainly *not* `unsigned`. But I'm not sure whether when the lecturer said, "use unsigned" he means "use `unsigned`" (which is what James has responded to), or "use unsigned types" (of which `size_t` is one). Then the stage 2 argument is, should you use `size_t` for sizes? I think it's fine, but some people (including Stroustrup IIRC) don't always bother, and others actively avoid it. – Steve Jessop Aug 30 '12 at 12:20
  • @Leo I don't tend to use that style in my programs really but was just pointing out a reason to use for it :-) Most compilers will warn to be sure, but some people ignore warnings and if it helps inexperienced developers to immediately spot errors then it's not such a bad thing. I think a list of reasons for the rules as Konrad said should have been provided by the lecturer – Firedragon Aug 30 '12 at 12:25
  • I'm not really aware about the references you talk about, but Strustroup invented and maintaining the language and probably not maintaining systems with millions of lines of code written in it, I think. And examples about languages for beginners or about features doesn't necessarily involve best practices because they may hide the point of the example. – TWiStErRob Aug 30 '12 at 12:40
  • 17
    @James Kanze, consider the purpose. You are comparing code produced by an experienced programmer to instructional examples. These rules are provided by the lecturer because they are the kinds of mistakes he sees his students make. With experience, the students can relax or disregard these absolutes. – Joshua Shane Liberman Aug 30 '12 at 13:11
  • 2
    @Component10 In the end, you have to find a consensus. The problem is that the semantics of `unsigned` in C++ don't work for numerical values, even when the actual values cannot be negative. Expressions like `abs( a - b )`, for example, give wrong results. – James Kanze Aug 30 '12 at 13:48
  • 1
    @KonradRudolph :-) Yes. Worse than the actual points is that there is no reason given. All rules should come with a rationale. – James Kanze Aug 30 '12 at 13:50
  • 1
    @ta.speot.is The fact that `size_t` must be an unsigned integral type is a historical wart on the language. Part of the motivation for requiring it to be unsigned is, in fact, linked to the problems with unsigned. If the signedness were to be made implementation dependent, it would be even worse. (And on 16 bit machines, you didn't want to require it to be more than 16 bits, even though objects with a size greater that 32K could, and did exist.) – James Kanze Aug 30 '12 at 13:53
  • 1
    @TWiStErRob Stroustrup invented the language _because_ he had to build and maintain large programs. (At least in part.) – James Kanze Aug 30 '12 at 13:55
  • 3
    @JoshuaShaneLiberman Some of them (like the suggestion of using `unsigned`) are bad rules, especially for beginners. I know the rules of integral promotion well enough that I can usually avoid `unsigned`s traps, but a beginner should _only_ see two numeric types: `double` and `int`. (Formally, `bool` and `char` are also numeric types, but the beginner should not see them as such: `bool` should only take the values `true` and `false`, and `char` should always contain characters.) – James Kanze Aug 30 '12 at 13:58
  • @SteveJessop The rule is to actively avoid unsigned integral types. I'm actually less strict about this than most experts; in limited contexts, where I'm mostly just comparing with `std::vector<>::size()`, for example, I'll use `size_t`. But the general rule is that if it makes sense to compare the magnitude of the difference between two values, then you **must** avoid all unsigned integral types. `abs( a - b )` will _not_ give_ you the results you expect. – James Kanze Aug 30 '12 at 14:36
  • 1
    I see the point, but whenever it's argued I wonder whether defining a `distance` function would after all be such an apocalyptic catastrophe. I'd certainly back a scheme where `abs` on an unsigned type gives an error or at least a warning. But AFAIK the ship has sailed on that: C has way too many implicit conversions, C++ introduced overloads of `abs`, so you can't stop people writing `abs(a-b)`. Modulo 2^n arithmetic is *never* what you want except hashes, and undefined behaviour on overflow is *never* what you want, so all of C++'s integer types are fundamentally broken one way or another. – Steve Jessop Aug 30 '12 at 14:53
  • 7
    I'd like to see this answer deleted, as most of the answer and ensuing discussion has nothing to do with the original question. It's noise. – Kristopher Johnson Aug 30 '12 at 16:56
  • 1
    @SteveJessop This issue likely is more one of implicit conversions than anything else. But the fact that subtraction of unsigned types returns an unsigned type is also a problem. And the arguments for using unsigned are really largely arguments for having subrange types. Which might not be a bad idea either, but it's not in C++ (nor, as far as I know, has it been proposed). – James Kanze Aug 30 '12 at 18:59
  • 8
    @Kristopher No, it’s not noise, it shows a very important aspect: namely that, regardless of what we may think about individual points on the list, compiling such a list as dogma is unanimously seen as bad. Arguably, that’s *the* answer to this question. – Konrad Rudolph Sep 01 '12 at 09:07
  • 2
    I agree with everything but `To experienced C and C++ programmers, the use of unsigned signals bit operators` – Qix - MONICA WAS MISTREATED Aug 01 '13 at 09:48
  • I just have to say: WTF delete this ??? never on earth do that. Try no never use raw pointers, unique_ptr is much better. If do raw pointers, then delete and assign to nullptr is helps finding bugs because operations on those pointers give you proper segfaults instead of silently pretending to work. – Arne Aug 06 '14 at 11:46
  • 2
    @Arne It's not clear what your point is. Most pointers in a well written application will be raw pointers, since most pointers will be for navigation (and `std::unique_ptr` can't be used for navigation). And assigning `nullptr` to something you've just deleted is usually a waste of time; usually, the `delete` will be in a context (e.g. a destructor) where the pointer will cease to exist afterwards anyway, and setting one pointer to `nullptr` doesn't do anything about the other pointers to the object. – James Kanze Aug 06 '14 at 12:47
  • @Arne And with regards to `delete this`, whether it's appropriate depends on the application. (With strong transactional integrity, for example, the `delete` may have to wait until the end of the transaction.) Still, the normal OO philosophy would be that objects themselves have intelligence, and manage their own lifetimes, deleting themselves in response to some external event. – James Kanze Aug 06 '14 at 12:49
  • Just throwing this out there; there's a lesser used, but still standard, type called `ssize_t` which is *signed `size_t`*. – Qix - MONICA WAS MISTREATED Apr 01 '16 at 18:48
46

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. But that would still be less readable than the code without any curly brackets.

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

Konrad Rudolph
  • 482,603
  • 120
  • 884
  • 1,141
  • +1 for a reasonable competing argument. However I tend to disagree. It's odd perhaps but I find your matrix example much harder to read without the extra brackets. I can sort of see the first looking better given that the throw would kick you out of the function but for most examples I would want the brackets. I have also seen where the first line of a function is a conditional and followed by a 'return' and that isn't so bad either. But the use of it vs the possibility of errors I think is just too great. – Firedragon Aug 30 '12 at 12:33
  • 27
    If you don't believe in writing code that takes up screen space unnecessarily, then you have *no business* putting the opening brace on its own line. I'm probably now going to have to duck and run from the holy vengeance of GNU, but seriously -- either you want your code to be vertically compact, or you don't. And if you do, don't do things designed solely to make your code less vertically compact. But as you say, having fixed that, you'd still *also* want to remove redundant braces. Or perhaps just write `if (a == nullptr) { throw null_ptr_error("a"); }` as one line. – Steve Jessop Aug 30 '12 at 12:52
  • FWIW, in the matrix example I personally would leave in the braces, with the open brace on the same line as the `for` and the close braces each on their own line. But I wouldn't have a blank line after the loop, and I *might* not have one before it either, so I'd use at most one line more than your preferred style. – Steve Jessop Aug 30 '12 at 13:02
  • 6
    @Steve As a matter of fact, I *do* put the opening brace on the previous line, for the very reason you stated. I used the other style here to make it more obvious how extreme the difference can be. – Konrad Rudolph Aug 30 '12 at 13:03
  • 9
    +1 I completely agree that your first example is much easier to read without braces. In the second example, my personal coding style is to use braces on the outer for-loop and not on the inner. I disagree with @SteveJessop about having to be one extreme or the other about vertically compact code. I omit extra braces with one-liners to reduce vertical space, but I do put my opening braces on a new line because I find it easier to see the scope when the braces are lined up. The goal is readability, and sometimes that means using more vertical space, other times it means using less. – Travesty3 Aug 30 '12 at 13:15
  • 1
    I wouldn't say that your first example is "horrible", but I do find it _slightly_ less readable than without the braces. When there's nesting, it's probably more readable with the braces, but again, with or without are not serious issues, provided you are consistent. The important point, in fact, is probably that the rule doesn't make sense in isolation. There are several ways of writing readable C++ code, based on a combination of indentation, braces and a number of other things. You decide on one, and stick to it. But the consideration is for a total style, and not any isolated element. – James Kanze Aug 30 '12 at 14:03
  • 22
    "I’ve never encountered this problem in real life": lucky you. Things like this don't just burn you, they give you 90% third degree burns (and that's just a few layers of management demanding a fix late in the evening). – Richard Aug 30 '12 at 15:16
  • 9
    @Richard I simply don’t buy that. As I’ve explained in the chat, even if this error should ever occur (which I find unlikely) it’s trivial to fix once you look at the stack trace because it’s obvious where the error is just by looking at the code. Your exaggerated claim is completely is completely baseless. – Konrad Rudolph Aug 30 '12 at 15:28
  • 1
    "I’ve never encountered this problem in real life": @Richard is right - this is the kind of things that tend to show at the most tedious moment. – Thorbjørn Ravn Andersen Aug 30 '12 at 19:04
  • 1
    If you write code omitting the braces and that code is modified by others and some of those programmers are not as attentive as you, I guarantee at some point the mistake will be made. Maybe you won't have to deal with it because you never make the mistake, but if you can help prevent future bugs by typing two extra characters, why not? – Brian Stormont Sep 01 '12 at 02:17
  • 3
    @Brian The point is not that *I* don’t make this mistake. The point is that I simply do not believe that *any* experienced programmers make this mistake, or, *if* they make it, that they catch it immediately and that it does no harm. Above, Richard even claims that this will result in a lot of hard work. I find this statement utterly incredible. – Konrad Rudolph Sep 01 '12 at 09:04
  • I don't always use braces for the reasons outlined above. The answer was given in the context of question: "what is gained by this rule". The "real world" example cited in my answer wasn't code I wrote, but a colleague's. If you use intendation properly it's very hard to get yourself into a position where you unknowingly change braceless flow control statements. – ta.speot.is Sep 02 '12 at 12:13
  • @ta.speot.is Fair enough. Luchian has actually told me much the same in chat so I think it’s only fair to remove your names from my answer. It’s not a personal accusation, after all. Cheers. – Konrad Rudolph Sep 02 '12 at 12:25
  • I don't have any problem with my name in your answer, especially as a reference to my answer. Thank you for the consideration, anyway. – ta.speot.is Sep 02 '12 at 12:41
  • 3
    [`goto fail`](http://www.zdnet.com/apples-goto-fail-tells-us-nothing-good-about-cupertinos-software-delivery-process-7000027449/) – Konrad Borowski May 25 '14 at 18:34
  • @xfix Yes, `goto fail` is the perfect contra-example to this answer. However, I must agree with Konrad Rudolph that the fully braced code is very ugly. Personally, I go with neither style, but put one statement bodies directly after the `if()` or `for()` *on the same line*. This prevents bugs like `goto fail` and is the most concise way to write such code. – cmaster - reinstate monica May 25 '14 at 19:33
  • 1
    @xfix Actually, the “goto fail” would not at all have been prevented by using braces. Admittedly even *I* thought that at first, but if you think about it for a bit – no. The `goto fail;` line was probably added by a version control merge or some similar process – even in braced code it could have gone *after* the brace, causing the exact same problem. The real lesson here is: use proper warning levels; *all* modern compilers warn about this code. This should never have passed review. – Konrad Rudolph May 25 '14 at 22:07
  • @SteveJessop: If each `if`, `while`, etc. is followed either by an indented single statement or by a `{` whose indentation matches both the control statement and the matching `}`, then single-statement control blocks will be readily recognizable as such. Moving `{` to the previous line makes it harder to visually confirm code is actually structured as intended. I would posit that putting each `{` in the same line or column as its `}` adds one line to complex control structures, but allows one to shave one line from simple ones. For code with more simple control structures, that's a win. – supercat Jul 16 '15 at 20:54
41

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.

jam
  • 3,500
  • 5
  • 31
  • 49
  • 17
    Isn't the problem in this example in the improper indentation and too long lines rather than the braces? – Honza Brabec Aug 30 '12 at 11:42
  • 7
    Yes, but following design/coding guidelines which are only 'safe' assuming that people are also following other guidelines (such as not having too-long lines) seems to be asking for trouble. Had the braces been in from the start, it would be impossible to end up with an incorrect if-block in this situation. – jam Aug 30 '12 at 12:05
  • 16
    How would adding braces (making it `if(really long...editor){ do_foo;}` help you avoid this case? Seems like the problem would still be the same. Personally I prefer to avoid braces when not necessary, however that has nothing to do with the time needed to write them but the reduced readability due to the two extra lines in the code. – Grizzly Aug 30 '12 at 21:02
  • 1
    Good point - I was assuming that enforcing use of braces would also result in them being put in a sensible place, but of course someone determined to make things difficult could put them in-line as in your example. I'd imagine most people would not, though. – jam Aug 31 '12 at 06:54
  • 2
    The first and last thing I do when touching a file is hit the auto-format button. It eliminates most of these issues. – Jonathan Allen Aug 31 '12 at 07:04
  • If it takes a whole second to make braces you should go to typing class. /s – Qix - MONICA WAS MISTREATED Apr 01 '16 at 18:50
20

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.

Nemanja Trifunovic
  • 23,597
  • 3
  • 46
  • 84
  • 5
    +1 just for the last point alone. A raw pointer has no business owning memory anyway. – Konrad Rudolph Aug 30 '12 at 12:23
  • 2
    With regards to using unsigned: not only Stroustrup, but K&R (in C), Herb Sutter and (I think) Scott Meyers. In fact, I've never heard anyone who truly understood the rules of C++ argue for using unsigned. – James Kanze Aug 30 '12 at 14:17
  • 2
    @JamesKanze In fact, on the same occasion I heard Stroustrup's opinion (a Boston conference in 2008), Herb Sutter was there and disagreed with Bjarne on the spot. – Nemanja Trifunovic Aug 30 '12 at 14:29
  • @NemanjaTrifunovic Then he's changed his opinion (or I got him mixed up with someone else). I can't get to Google Groups from here, and I'm not sure what search criteria to use, but I know that there was a big discussion about it in `comp.lang.c++.moderated` some time back, and in the end, the consensus was against using unsigned. Including people who'd originally argued for it. (Part of the consensus was that you avoid it because `unsigned`, as defined in C++, is broken. I wouldn't go that far, but it's semantics do limit its usability.) – James Kanze Aug 30 '12 at 14:49
  • 3
    Just to complete the "`unsigned` is broken", one of the problems is that when C++ compares similar sized signed and unsigned types, it converts to the unsigned one **before** doing the comparison. Which results in a change in value. Converting to the signed wouldn't necessarily be much better; the comparison should really take place "as if" both values were converted to a larger type which could represent all of the values in either type. – James Kanze Aug 30 '12 at 14:56
  • @NemanjaTrifunovic I just did some searching on the net. It was probably Scott Meyers I was thinking of, and not Herb Sutter. See http://www.aristeia.com/Papers/C++ReportColumns/sep95.pdf – James Kanze Aug 30 '12 at 15:02
  • @James: That certainly establishes that Meyers is against using unsigned types, but I find his last conclusion extraordinary, that no function should ever return a value greater than `INT_MAX` because it is (he says) essential to cater to clients who assign it to an `int` when `int` is not a large enough type for the result. As such, I'm sure "no `unsigned`" is his position but I wonder whether that paper truly is his considered final argument on the subject. Is there *anything* a client might do with a number that is so stupid it's not my fault as the designer of the interface? – Steve Jessop Aug 30 '12 at 15:56
  • I guess to be fair, the paper was written in 1995, in the sweet spot when 16 bit `int` was behind us and 64 bit `long` was ahead. "Us" being "people who use proper computers". So perhaps all mentions of `int` should be taken as if they were `intmax_t` in modern code, and then the conclusion holds that you shouldn't aim to use values beyond `INTMAX_MAX` just for the sake of extending the range your program handles. – Steve Jessop Aug 30 '12 at 16:04
  • 1
    @SteveJessop I think you have to take it in the context of a function returning `unsigned`. I'm sure he has no problem with `exp(double)` returning a value more than `MAX_INT`:-). But once again, the real problem is implicit conversions. `int i = exp( 1e6 );` is perfectly valid C++. Stroustrup actually proposed deprecating lossy implicit conversions at one point, but the committee wasn't interested. (An interesting question: would `unsigned` -> `int` be considered lossy. I'd consider both `unsigned` -> `int` and `int` -> `unsigned` lossy. Which would go a long way to making `unsigned` OK – James Kanze Aug 30 '12 at 19:05
  • 1
    `Never have a pointer which points to a deleted or non-existing object.` How do you do this without changing the pointer after the object's deletion? – harper Dec 30 '12 at 16:28
19

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.

Alok Save
  • 190,255
  • 43
  • 403
  • 518
14

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.

Component 10
  • 9,649
  • 4
  • 42
  • 58
  • 5
    I once saw some documentation that had the wonderful coinage "Microsoftligent". Yes, it's possible to make significant mistakes with global search and replace. That just means that global search and replace has to be used intelligently, not microsoftligently. – Pete Becker Aug 30 '12 at 12:27
  • 4
    I know this isn't my post-mortem to perform, but if you're going to do text replacement on source code then you should do it according to the same rules you'd use for the kind of text replacement that's well-establised in the language: macros. You shouldn't write a macro `#define FOO() func1(); \ func2(); ` (with a linebreak after the backslash), same goes for search and replace. That said, I have seen "always use braces" advanced as a style rule precisely because it saves you from wrapping all your multi-statement macros in `do .. while(0)`. But I disagree. – Steve Jessop Aug 30 '12 at 12:29
  • 2
    Btw, that's "well-established" in the sense that Japanese knotweed is well-established: I'm not saying we should go out of our way to use macros and text-replacement, but I'm saying that when we do such a thing, we should do it in a way that works, rather than doing something that only works if a particular a style rule was succesfully imposed on the whole code base :-) – Steve Jessop Aug 30 '12 at 12:32
  • @SteveJessop: You're right, of course, and good point about using macro rules. Also agree that the tail shouldn't wag the dog. `sed` is clearly not the best tool to perform context based refactoring on C++ code. That said, there were no more appropriate tools available at the time and, if I'm honest, I learned a great amount from the experience, such as the above - you learn a lot from your mistakes. One of the biggest lessons: Do all you can to protect your code, and therefore your reputation from future meddlers (such as me!) – Component 10 Aug 30 '12 at 13:17
  • 1
    @SteveJessop One could also argue for braces and a belt. If you have to use such macros (and we did, before C++ and `inline`), then you should probably aim for them to work as much like a function as possible, using the `do { ... } while(0)` trick if necessary (and lots of extra parentheses. But that still wouldn't stop you from using braces everywhere, if that's the house style. (FWIW: I've worked in places with varying house styles, covering all of the styles discussed here. I've never found any to be a serious problem.) – James Kanze Aug 30 '12 at 14:11
  • @James: agreed, I would certainly be defensive about macros even if I was also defensive about putting braces everywhere in code, it's just another position I've encountered. I think a lot of arguments about the readability and error-proneness of different styles unintentionally come from people seeing something surprising and assuming it's bad. Any style that's not completely stupid will hold up provided everyone knows what it is, because they'll also know to keep an eye out for its gotchas. The biggest problems are when (e.g.) someone who's used to braces everywhere misreads code without. – Steve Jessop Aug 30 '12 at 14:25
  • 2
    And I reckon, the more styles you've worked with the more carefully you read and edit code. So even if you have a preference what's easiest to read, you'll still successfully read the others. I worked in a company where different components were written in different "house styles" by the different teams, and the correct solution is to complain about it in the lunch room to no great effect, not to try to create a global style :-) – Steve Jessop Aug 30 '12 at 14:28
  • Thankfuly my changes were permanent ones rather than macro driven pre-processor time substitutions. Same rules still apply though. I did propose to the client an inline wrapper function to do the conversion which would have been much less risky and faster to implement, but this was overruled on the grounds that it would would be unecessarily confusing to future maintainers, who need not be aware of the original API. I digress though - I used the example mainly to demonstrate that automatic conversion of one to many instructions can break code if carelessly done. – Component 10 Aug 30 '12 at 14:28
8

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();
    }
}
Lukasz Madon
  • 13,748
  • 13
  • 58
  • 93
  • 1
    Just use commas in the `using` statement and you don't have to :) – Ry- Aug 30 '12 at 15:34
  • 1
    @minitech That simply doesn’t work here – you can only use the comma when the types are equal, not for unequal types. Lukas’ way of doing this is the canonical way, the IDE even formats this differently (note the lack of automatic indentation of the second `using`). – Konrad Rudolph Sep 01 '12 at 09:11
8

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).

KeithS
  • 65,745
  • 16
  • 102
  • 161
  • 3
    @AlexBrown ...which was exactly my point. The rule as stated in the OP is "always use brackets, even for single lines", which I disagree with for the reason I stated. Brackets *would* help with the very first code example, because the code will not behave the way it's indented; you'd have to use brackets to pair the `else` with the first `if` instead of the second. Please remove the downvote. – KeithS Aug 30 '12 at 15:19
5

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;
Konrad Borowski
  • 9,885
  • 2
  • 50
  • 68
  • Exactly. Just don't put the (completely unnecessary) newline + indent, and you completely sidestep this issue that everyone is always so quick to bring up. – Alexander May 02 '18 at 16:25
4

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

Drona
  • 6,117
  • 1
  • 26
  • 35
4

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

Tom Tanner
  • 8,967
  • 2
  • 28
  • 58
  • 6
    If you happen to go through that path twice you've got a programming error. Setting a pointer to null to make this error less harmful doesn't solve the underlying problem. – Pete Becker Aug 30 '12 at 12:30
  • 1
    Agreed, but I have seen this recommended before, and I believe it's in some professional programming standards. I was commenting more on why the poster's professor had come up with it, rather than when it was any good – Tom Tanner Aug 30 '12 at 12:38
  • 2
    Following up to what Pete Becker said: it doesn't solve the underlying problem, but it may mask it. (There are cases where you would set a pointer to `NULL` after deleting it. If `NULL` is a correct value for the pointer to have in those circumstances; e.g. the pointer points to a cached value, and `NULL` indicates an invalid cache. But when you see someone setting a pointer to `NULL` as the last line in a destructor, you wonder if he knows C++.) – James Kanze Aug 30 '12 at 14:20
4

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++;
}
MikeFHay
  • 7,264
  • 4
  • 24
  • 43
  • 7
    I like how everyone manages to ignore `for (int i = 0; i < 100; i += 2);`, for the sake of continuing the argument about indentation ;-) There's probably a whole separate bunfight we could have, how "best" to express the logic "for each `i` in a certain range with a certain property" in C++ without a loop, using some nightmare combination of standard algorithms, `filter_iterator` and/or `counting_iterator`. – Steve Jessop Aug 30 '12 at 14:35
  • 1
    Also, if we had that then we could disagree about how to indent the resulting massive single statement. – Steve Jessop Aug 30 '12 at 14:43
  • 2
    @Steve, It's just an example though. There are plenty of legitimate uses of the pattern. Obviously if you want to count the numbers from 1 to 100 which are divisible by 2, all you have to do is 100/2. – MikeFHay Aug 30 '12 at 15:21
  • 2
    Sure, I know, that's why I abstracted to "for each `i` in a certain range with a certain property". It's just that usually on SO, people are very quick to ignore the actual question in favour of a completely different approach to the example given. But indenting is *important*, so we don't ;-) – Steve Jessop Aug 30 '12 at 15:45
4

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
Blacktiger
  • 1,235
  • 1
  • 9
  • 19
  • 5
    The second option would yield a compilation error, because the `else` is not associated with an `if`. – Luchian Grigore Aug 30 '12 at 15:09
  • One not so visible problem with inline is that most IDEs on default change it to the indented style when using their autoformat utility. – Honza Brabec Aug 30 '12 at 16:30
  • @Honza: that's a highly charged political issue, though. If we're co-operating on a code base, either we have to use the same indenting style down to every last detail, or we have to both agree not to autoformat existing code "just because". If the former then the agreed style could still include this, but you'd have to either configure your IDE to respect it or not use autoformat. Agreeing that the common format is "whatever my IDE autoformats to" is all very well if we all use the same IDE forever, not so good otherwise. – Steve Jessop Aug 30 '12 at 17:59
4

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.

Community
  • 1
  • 1
Pureferret
  • 6,477
  • 14
  • 72
  • 149
  • 1
    (1) Putting the statement on the same line makes it *less* readable, not more. Especially simple thinks like an increment are easily overlooked. *Do* put them on a new line. (2) Of course you can put your `for` loop on a single line, why shouldn’t this work? It works for the same reason that you can omit the braces; newline is simply not significant in C++. (3) Your conditional operator example, besides being horrible, is invalid C++. – Konrad Rudolph Sep 03 '12 at 13:01
  • @KonradRudolph thanks, I'm a bit rusty at C++. I never said (1) was more readable, but it would signal to be that that piece of code was *meant* to be online one line. (2) My comment was more that I wouldn't be able to read it and know it worked, whethert at all or as intended; it's an example of what not to do for this reason. (3) Thanks, I've not written C++ in a long time. I'll fix that now. – Pureferret Sep 03 '12 at 13:26
  • 2
    Also putting more then one expression in one line makes it harder to debug code. How do you put breakpoint on 2nd expresion in that line? – Piotr Perak Sep 04 '12 at 20:07
3

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.

P.P
  • 106,931
  • 18
  • 154
  • 210
3

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.

Artemix
  • 1,899
  • 2
  • 21
  • 32
  • If your fast coder cannot be bothered to spot a syntax-highlighted `return` statement within the body of a function before adding something to it, you shouldn't let the fast coder get near your code. You won't stop such a guy from trolling around your code by including braces. – cmaster - reinstate monica May 25 '14 at 19:51
  • @cmaster He doesn't work with us anymore. Anyway, syntax highlighting is good, but remember that there are persons that do not see clearly (I saw even a post from a blind programmer last year). – Artemix May 26 '14 at 09:11
2

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++;
RollRoll
  • 7,107
  • 17
  • 64
  • 118
1

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.

John
  • 2,999
  • 2
  • 14
  • 16
  • 2
    If there was a logic error, it should be fixed, rather that masking it. – uınbɐɥs Sep 04 '12 at 19:10
  • @Barmar, OP says... 6. Set Pointer to NULL after deletion - Double delete protection // not bad. Some people have responded on not setting it to Null and I am saying why it should be set to NULL, what part of 6. my answer to setting NULL doesn't fit in 6? – John Sep 08 '12 at 00:36
  • 1
    @Shaquin, And how do you propose in finding these logic errors in the first place? Once you set the pointer variable to NULL after memory has been deleted. Any attempt to reference NULL pointer will crash to the debugger on the line your illegal attempt was made. You can trace back and see where the logic error was and fix the problem. If you do not set the pointer variable to NULL after you have deleted the memory, your illegal attempt to write this deleted memory due to UNAWARE logic errors may succeed and therefore does not crash at that point. It is not masking it. – John Sep 08 '12 at 01:20
1

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.

ST3
  • 8,050
  • 2
  • 62
  • 85
1

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.

Öö Tiib
  • 9,482
  • 23
  • 37
1

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.

supercat
  • 69,493
  • 7
  • 143
  • 184