• Post Reply Bookmark Topic Watch Topic
  • New Topic

Why you should always use curly braces  RSS feed

 
fred rosenberger
lowercase baba
Bartender
Posts: 12562
49
Chrome Java Linux
  • Likes 6
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I know I've posted it scores if not hundreds of times. But you should always use curly braces when writing your If-statements, even if not strictly necessary.

As Apple found out.
 
Bear Bibeault
Author and ninkuma
Marshal
Posts: 66304
152
IntelliJ IDE Java jQuery Mac Mac OS X
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
That's gotta hurt!
 
Winston Gutkowski
Bartender
Posts: 10575
66
Eclipse IDE Hibernate Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
fred rosenberger wrote:... you should always use curly braces when writing your If-statements, even if not strictly necessary.
As Apple found out.

Double whammy. No braces AND gotos. You reap what you sow.

Winston
 
Scott Winterbourne
Ranch Hand
Posts: 116
2
Eclipse IDE Java PHP
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Oh wow that is crazy.

I've actually had some colleagues recently push not using curly braces on short if statements like this. I always use curly braces just so I can quickly see which code block belongs to which if statement. I think I will stick with this convention.
 
J. Kevin Robbins
Bartender
Posts: 1801
28
Chrome Eclipse IDE Firefox Browser jQuery Linux MySQL Database Netbeans IDE
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I resent the comment that "everyone uses GOTO's". I haven't used one since GWBASIC back in the 1980's. The programmer who wrote that code should be demoted to burger flipper.
 
Winston Gutkowski
Bartender
Posts: 10575
66
Eclipse IDE Hibernate Ubuntu
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
J. Kevin Robbins wrote:The programmer who wrote that code should be demoted to burger flipper.

Unless of course he's now VP of operations; in which case he'll get a 7-figure golden handshake and be working at Google next month.

Winston
 
Matthew Brown
Bartender
Posts: 4568
9
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I always add the braces. I've seen one person give a convincing explanation why they don't, though. I was at a Robert Martin (Uncle Bob) talk a few months ago, and he was explaining how his aim is to reduce all conditionals and loops by refactoring to the point where they contain a single statement. Which means that the need for braces is actually an indication that you haven't finished refactoring yet.

Gotos are unforgivable, though .
 
Jayesh A Lalwani
Rancher
Posts: 2762
32
Eclipse IDE Spring Tomcat Server
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
OOH.. I'll svn blame on that code the moment I see it.
 
Scott Winterbourne
Ranch Hand
Posts: 116
2
Eclipse IDE Java PHP
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
It blows my mind that something like this made it to production. I know it's not possible to catch every possible bug under the sun but this seems blatent enough that a unit test would have caught it. Even if it made it beyond the unit test, a test case by the QE's should have caught it.

A case of cutting corners to make a deadline perhaps?
 
Jan Hoppmann
Ranch Hand
Posts: 147
Android Eclipse IDE Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
All our company projects and my private home projects use the same formatter for Eclipse - it adds the braces automatically when you save your file. If something goes wrong while testingafterward, it normally takes about 2 minutes to spot in the debugger ;)
 
Mike Simmons
Ranch Hand
Posts: 3090
14
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Well, with auto-formatting this would have been a non-issue anyway - the indentation would have been corrected to expose the bug. To the extent it wasn't already obvious to anyone bothering to look at it anyway - the two identical gotos right next to each other seem like a huge red flag to me. But someone still has to look at it and engage a brain cell or two to realize there's a problem.

I would think that any decent IDE should also be able to flag the code as suspicious. At least, in the Java world it would. Of course we don't have goto, but even so, in most Java variations of this problem that I can come up with, IntelliJ will flag something as red or at least yellow, suggesting there's something illogical about the code structure. Are IDEs for C that far behind in what problems they can detect? Or does this just indicate the programmer isn't using modern tools?

I don't really think the coding style, IDE, or even the gotos, are the main issue here though. The lack of testing is. Both in terms of unit tests and QA. I'm with Scott on that one.

Incidentally, the fix is finally available on OS-X. Fellow Mac users, update now.

Testing to see whether you're vulnerable is available at:

https://gotofail.com
 
Ryan McGuire
Ranch Hand
Posts: 1143
9
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Mike Simmons wrote:Well, with auto-formatting this would have been a non-issue anyway - the indentation would have been corrected to expose the bug. To the extent it wasn't already obvious to anyone bothering to look at it anyway - the two identical gotos right next to each other seem like a huge red flag to me. But someone still has to look at it and engage a brain cell or two to realize there's a problem.


Then again, if you use Python, where the indentation matters, this would have been a non issue.



is different from



Disclaimer: I know just enough Python to know that indentation matters. I don't even pretend that the code above is 100% correct.


I don't really think the coding style, IDE, or even the gotos, are the main issue here though. The lack of testing is. Both in terms of unit tests and QA. I'm with Scott on that one.


Absolutely! Even if Apple didn't have automated unit testing of the code they should have had some test to verify that the output of SSLHashSHA1.final() matters. As it was, that code never got executed.



 
Anayonkar Shivalkar
Bartender
Posts: 1558
5
Eclipse IDE Java Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I read about this 'goto fail' but bug few days back.

I have a question (call it stupid ) - how did this source code got out in public? Is it copyright infringement, or Apple has released this code to public?
 
Winston Gutkowski
Bartender
Posts: 10575
66
Eclipse IDE Hibernate Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Ryan McGuire wrote:Then again, if you use Python, where the indentation matters, this would have been a non issue.

Oddly enough, that was my immediate reaction to the post.

I probably shouldn't say this, but I generally don't write braces for single line sub-blocks on if statements (at the risk of getting flamed, I find unnecessary braces on them "fussy" and "noisy"); but I do indent my code. ALWAYS. It just doesn't feel right if I don't, and it's one of the first things I set up when I'm working with an IDE - and I use Ctrl+Shift+F (Eclipse) constantly.

That's not to say that I didn't miss it several times when I was testing for the SCJP exam (a few years back now), so I heartily back Fred's recommendation.

Do as I say; not as I do.

Winston
 
Paul Clapham
Sheriff
Posts: 22819
43
Eclipse IDE Firefox Browser MySQL Database
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Anayonkar Shivalkar wrote:Is it copyright infringement, or Apple has released this code to public?


It doesn't have to be either. Quoting a small fragment of a copyrighted work is often legal -- for example if you read a book review you may see quotes from the book in the review. I think there's also a list of purposes which you can put the small fragments to, and I think that "education" is one of them. Which would apply here.

It's also the case that copyright law varies from place to place -- if I remember right the recent changes to Canada's copyright laws included "satire" as an acceptable reason for publishing fragments of a copyrighted work. (Which doesn't apply here.)
 
Piet Souris
Master Rancher
Posts: 2041
75
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I try not to be too dogmatic about using curly braces or not, and using the occasional goto.
Ever since using C I never liked these curly braces. I find them ugly. Always missed my trusty
Basic:

So when the statement is just one statement, I don't use braces, like in:

And although I never use GOTO (not even in Basic) I do not hesitate to use
'continue loop_n;' when I'm in a deeply nested loop.
 
Mike Simmons
Ranch Hand
Posts: 3090
14
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
The source code for this bug was already open-sourced by Apple and published here.
 
Anayonkar Shivalkar
Bartender
Posts: 1558
5
Eclipse IDE Java Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Thanks Paul & Mike!
 
Stephan van Hulst
Saloon Keeper
Posts: 7961
143
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Winston Gutkowski wrote:I probably shouldn't say this, but I generally don't write braces for single line sub-blocks on if statements (at the risk of getting flamed, I find unnecessary braces on them "fussy" and "noisy"); but I do indent my code. ALWAYS.


I'm happy you did. I experience myself to be a bit of a lone ranger out there. I write so many single line if/for/while statements that my formatting options for these statements are set to "eliminate", rather than "generate". (my do-whiles however are set to "generate". Not that it matters much, because I have my editor warn me whenever it encounters the keyword do )

I put lots of empty lines in my code. It gives me peace of mind. Closing braces, however, make me restless.
 
Winston Gutkowski
Bartender
Posts: 10575
66
Eclipse IDE Hibernate Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Stephan van Hulst wrote:I write so many single line if/for/while statements that my formatting options for these statements are set to "eliminate", rather than "generate".

And that's where we differ, and why Piet's absolutely right: there's no point in being dogmatic about it. I always put braces on loops, even if they're empty, simply because I find:

while(some condition that probably includes an increment);

too easy to misread (old eyes and Alzheimer's ).

Winston
 
Stephan van Hulst
Saloon Keeper
Posts: 7961
143
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I personally consider empty loops to be a code smell. I have empty statements set as an error. If there is a nested increment in the loop header, I tend to move it to the body, even if that makes the code more verbose.
 
Martin Vajsar
Sheriff
Posts: 3752
62
Chrome Netbeans IDE Oracle
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Winston Gutkowski wrote:I probably shouldn't say this, but I generally don't write braces for single line sub-blocks on if statements (at the risk of getting flamed, I find unnecessary braces on them "fussy" and "noisy");

I'm doing the same for years. My main argument (besides habit) was "to see more code on the screen". However, recently I briefly worked on some project with he opposite convention (braces everywhere), and to my surprise I found that it is probably more convenient - adding a new line to a one-line block is so much easier this way. Consider me converted (I'd probably convert my other project to the new convention, but it's too many files to touch).

but I do indent my code. ALWAYS. It just doesn't feel right if I don't, and it's one of the first things I set up when I'm working with an IDE - and I use Ctrl+Shift+F (Eclipse) constantly.

So do I. Unlike the braces, I don't see any valid argument to do otherwise. However, I generally don't do it using automatic formatters. Perhaps another habit I should get rid off.
 
Ryan McGuire
Ranch Hand
Posts: 1143
9
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
At the cost of reviving a dead thread...

Stephan van Hulst wrote:I personally consider empty loops to be a code smell. I have empty statements set as an error. If there is a nested increment in the loop header, I tend to move it to the body, even if that makes the code more verbose.

I consider your cure to be worse than the sickness. For instance, I could see something like the following:

Converting that to put the increment in the body...

...seems horrible to me. I've put the increment expression in the for() line millions of times. That's what I've grown up with and that's what I do now. Indeed, that is exactly what the third expression in the parentheses of a for() loop is for. Moving the increment expression to the body of the for() loop just because the body would be empty without it is bad reason to break an existing idiom that is so ingrained in, I would guess, everyone's mind.

Notice, however, that in the first chunk of code the semicolon for the empty statement is on the line below the for() line. That makes it that much easier to recognize the for() loop body as empty. Using an empty set braces on that second line would be my second choice:


 
Junilu Lacar
Sheriff
Posts: 11476
180
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Wouldn't you need to have the loop condition as (i < myArray.length && someCondition(myArray[i])) to account for the case where all elements satisfy the condition? Anyway, I would lean towards refactoring this with Extract Method to isolate the empty loop and hide that borderline ugliness. I prefer the empty code block with braces.

This is one case where I definitely put clarity above performance.
 
Ryan McGuire
Ranch Hand
Posts: 1143
9
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Junilu Lacar wrote:Wouldn't you need to have the loop condition as (i < myArray.length && someCondition(myArray[i])) to account for the case where all elements satisfy the condition?

In my hypothetical situation there was always going to be an non-conforming element, before the end of the array. But yes, putting int he explicit check is good idea anyways.

Anyway, I would lean towards refactoring this with Extract Method

I see that regardless of whether the code is inline or in a separate method, your i++ was in the for() loop parens instead of the body. ...which is exactly my point.
 
Junilu Lacar
Sheriff
Posts: 11476
180
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Yeah, I find the for-loop with the missing increment part less intuitive than one with an empty body but I wouldn't go so far as to say it's "horrible"; seems a bit too strident of an objection to me. I wonder if logic like this could be expressed with Java 8 features more elegantly. I can almost imagine doing some kind recursive thing in Scala for it.
 
Aaron Shawlington
Ranch Hand
Posts: 50
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Braces aside, theres absolutely nothing wrong with that usage of goto in that snippet (apart from the bug of course).
The objections ('GOTOs are unforgivable' ... not 'GOTOs are typically a bad idea') sound like cargo cult coding to me.
 
Dave Tolls
Ranch Foreman
Posts: 3056
37
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Ryan McGuire wrote:
Junilu Lacar wrote:Wouldn't you need to have the loop condition as (i < myArray.length && someCondition(myArray[i])) to account for the case where all elements satisfy the condition?

In my hypothetical situation there was always going to be an non-conforming element, before the end of the array. But yes, putting int he explicit check is good idea anyways.


In my case, for this, I would:

I don't like making the assumption that we will find a suitable element. It makes me itch.


Besides, I've generally viewed a for-loop as a "loop through this set of things" so prefer to actually loop through it, and place any additional exit criteria in the loop itself.
I know there's arguments for it (eg, it's an exit criteria so it should be in the for statement), but I find it can get cluttered.
And frankly I'm a simpleton, and clutter messes with my mind.
 
chris webster
Bartender
Posts: 2407
36
Linux Oracle Postgres Database Python Scala
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I defer to my colleagues above on the question of using braces or not. But I'm with Dave on this particular example: a "for" loop is for looping through a known collection of things, so wherever possible I prefer code that works with the idiom rather than around it. In this case, why wouldn't you use an ArrayList and the indexOf method, instead of writing awkward for loops? Maybe the awkward loop is a code smell?

Of course, we still haven't addressed the thorny issue of whether to place the braces on the same line or not....
 
Junilu Lacar
Sheriff
Posts: 11476
180
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
chris webster wrote:Of course, we still haven't addressed the thorny issue of whether to place the braces on the same line or not....

I smell a style war brewing...

For the loop breakers out there, I would remind you that the for-statement is defined as:

for (initialization; termination; increment) {
    statement(s)
}

So if you're terminating the loop from the body, aren't you actually "working around" the idiom rather than with it? I prefer to actually do something with the current element in the loop body besides just checking it for the loop termination condition.

Lucky for me, I have the final word on my team and I get to say, "Our style on this will be: empty braces on the same line, blank line before is optional at your discretion, blank line after is mandatory. This makes you scan the line for the loop body, which is easy enough to find. No breaks from the loop body unless you're doing more in it but avoid them if you can. Exceptions can be discussed in code reviews."

If we're going to be honest though, it's like trying to put lipstick on a pig. Except my pig is prettier than your pig. Let the games begin!
 
Matthew Brown
Bartender
Posts: 4568
9
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Junilu Lacar wrote:
For the loop breakers out there, I would remind you that the for-statement is defined as:

for (initialization; termination; increment) {
    statement(s)
}

So if you're terminating the loop from the body, aren't you actually "working around" the idiom rather than with it? I prefer to actually do something with the current element in the loop body besides just checking it for the loop termination condition.


There's a difference between the definition and the idiom, though, isn't there? What proportion of for statements do you see where the termination condition is anything other than index < someExpression? Maybe 1%? Therefore many people don't look at it carefully, and so won't notice the real intent of the code. A break statement, now that's obvious.

Though playing with functional languages is making me think I don't want to use a loop at all - a nice higher-order find function with a lambda expression for the termination condition will do me fine .
 
Dave Tolls
Ranch Foreman
Posts: 3056
37
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Junilu Lacar wrote:
chris webster wrote:Of course, we still haven't addressed the thorny issue of whether to place the braces on the same line or not....

I smell a style war brewing...

For the loop breakers out there, I would remind you that the for-statement is defined as:

for (initialization; termination; increment) {
    statement(s)
}

So if you're terminating the loop from the body, aren't you actually "working around" the idiom rather than with it? I prefer to actually do something with the current element in the loop body besides just checking it for the loop termination condition.


I refer the honorable gentleman to the post I made earlier.</parliament>
;)

Or, put another way, fight fight fight...
 
Tim Holloway
Saloon Keeper
Posts: 18789
74
Android Eclipse IDE Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I can get quite hostile about braces myself.

I got sick and tired of having stuff like this happen when I insert temporary debugging code:



It's even worse when I yank out a line of code and forget it was conditional.

Never mind unit tests, Murphy says that this is something that comes out after 17 hours of an 18-hour batch process.
 
Jelle Klap
Bartender
Posts: 1952
7
Eclipse IDE Java
  • Likes 2
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Clearity and readability beats out demonstrating "cleverness" by not adding braces where they aren't strictly necessary.
Adding braces at all times prevents nasty bugs and it's nice and consistent. I don't have to waste any time worrying about whether braces were omitted intentionally or if I'm staring down a bug. Don't make me think.
 
Junilu Lacar
Sheriff
Posts: 11476
180
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Matthew Brown wrote:Though playing with functional languages is making me think I don't want to use a loop at all - a nice higher-order find function with a lambda expression for the termination condition will do me fine .

We can definitely call a truce in the Braces War if you come to the farmhouse and put that on the negotiation table.
 
Junilu Lacar
Sheriff
Posts: 11476
180
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Jelle Klap wrote:Clearity and readability beats out demonstrating "cleverness" by not adding braces where they aren't strictly necessary.

They're hardly my idea of "cleverness" which is why I don't really mind them that much. When I see them in other people's code, I don't instinctively cringe. Rather, I think "Ok, either everything that's noteworthy has already been done or there's nothing noteworthy to be done here. Let's see where the beef is... (backtrack over the code)" And as I said before, I would refactor this out to its own method to hide the borderline ugliness. That's how I'd make it clear.

Maybe it's just me but then again, this is what style wars are about, yeah?

But I'm with Matthew on Java 8 high-order functions and lambdas for the peace pipe and the olive branch -- sorry for mixing metaphors, don't want to start another war...
 
  • Post Reply Bookmark Topic Watch Topic
  • New Topic
Boost this thread!