7 ms·
If blocks without curly braces ಠ_ಠ
by badman_ting 13y ago
If blocks without curly braces ಠ_ಠ
- CrLf 13y agoThat's what I first thought too. The only situation where braces can be safely left out is when the single statement is on the same line as the "if".
- fidotron 13y agoInstant code review fail in my book too. Outside of python, for obvious reasons, it should never be allowed. The slightly nuts conditionals in Erlang are justifiable for avoiding exactly this kind of problem.
- fhd2 13y agoAgreed. Mandatory curlies might have helped avoid disaster here, but the code in question is still utter rubbish. If blatant nonsense makes it past review (or if there is no review), no coding style rule in the world is going to help.
- mkonecny 13y agoand goto statements everywhere :S
- jenandre 13y agothe gotos actually make sense in this case. unless you'd prefer some insane tree of if/else?
- oneeyedpigeon 13y agoCouldn't you just set a 'failed' boolean and wrap the fail: statements in a conditional check on it? Not that I'm saying all uses of goto, particularly this one, are necessarily absolute evil.
- voidlogic 13y agoI agree, I was thinking about what this situation would look like in other langs and when I turned to Go, I realized: While Go has goto for tricky situations like this, because it has defer you don't have to use it often, assuming the free calls were needed (and the vars were not going to be GC'd): defer SSLFreeBuffer(&hashCtx) defer SSLFreeBuffer(&signedHashes) if err = SSLHashSHA1.update(&hashCtx, &serverRandom); err != nil { return err; } else if err = SSLHashSHA1.update(&hashCtx, &signedParams); err != nil { return err; } else if err = SSLHashSHA1.final(&hashCtx, &hashOut); err != nil { return err; } return nil; }
- acdha 13y agoI would probably flag the code above in a security review because it hides the key part at the end of a complex line. Unless you're coding on VT100 terminal it's worth the extra line to make the test logic incredibly obvious: err = SSLHashSHA1.update(&hashCtx, &serverRandom); if (err != nil) { return err; }
- voidlogic 13y agoIn actual code things would not be named as they were above and it would be shorter, I was just trying to make it look reasonably like the C for HN.
- acdha 13y agoTrue, but I've definitely noticed that particular style of writing if tests using a one-line assignment and obscured test condition seems to be pretty common in the Go community and it's a bad habit for understanding code.
- voidlogic 13y ago>>it's a bad habit for understanding code. Is there objective evidence for this? As a Go programmer a semicolon in an if statement screams to me. I can see it possibly being in issue for new Go programmers- but I don't remember it being one for me.
- brodo 13y agoNo need for that. Just return early. Plus one should split up this gigantic function into several smaller ones.
- coldtea 13y agoThis is what goto statements are good for. The Linux kernel also uses goto statements a lot for similar reasons. This is not spaghetti code or what Dijkstra talked against. It's one of the two things one hears novices: that gotos are to always be avoided (because they heard that they are bad), and that we should write stuff in assembly (cause they heard that it's fast).
- Pacabel 13y agoI cringe whenever I hear people advocating that it's okay to avoid braces with single-statement conditionals/loops/etc. This is a perfect example of just how bad that suggestion is, and just how disastrous it can be. It's totally worth typing a few extra characters here and there to basically avoid these kind of situations completely.
- badman_ting 13y agoYes, precisely. It's a real "cowboy coder" tell if you ask me, it just reeks of "it's ok because I won't screw up". We should distrust ourselves.
- raverbashing 13y agoExactly this This also applies to the "JS without ;" crowd. You may think you're too good to know all the rules of ; or you can just don't think about it, and worry about other things instead, like your code.
- Tloewald 13y agoI hate omitting braces, but Javascript semicolon omission is a totally different argument. It's easy to show simple cases where omitting braces causes problems. It's actually quite difficult and artificial to find cases where (especially with "use strict") semicolons confer any benefit at all, and in some cases they make things worse (e.g. multi-line variable declarations). Also, "correct" use of semicolons in Javascript makes code less consistent (e.g. some function declarations end with a semicolon, others do not) whereas consistent use of braces is consistent. I certainly see far more bugs caused by an improperly inserted semicolon than an improperly omitted semicolon, but then I'm looking at jshinted code most of the time.
- mistercow 13y agoI used to believe as you do that semicolon issues were contrived and unlikely in JavaScript. But over the years I've been bitten by it enough times to know better. IIFEs are the most common source of semicolon problems: for(i = 0; i < 10; i++){ x = arr[i] (function(x) { setTimeout(function(){console.log(x)}) })() } A less common sort of pattern that I still use pretty often as a DRY measure: init() [x, y, z].forEach(function(a){ if(a.length > 5) { process(a) } }) This is clean and readable, and without semicolons it's completely wrong. Now sure, you can prepend semicolons to these specific cases, and that will work. Here's why I dislike that: 1. Those leading semicolons look weird. To the uninitiated, it looks like a typo, and that's a significant hazard if anyone else is going to touch your code. 2. While there are only a handful of special rules like this to remember, there's only one rule to remember if you don't rely on ASI.
- shurcooL 13y agoInteresting that, if there was a source code formatting tool (like gofmt for go), it would've made the bug easier to spot because the 2nd goto would lose it's misleading indentation.
- mitchty 13y agoThere is such a tool, clang-format in 3.4 does as you expect. This is what happens if you try the same thing in it http://imgur.com/syt6LuU http://imgur.com/syt6LuU. I have emacs setup to auto run clang-format on each save of cc-mode files. I can't NOT notice this stuff now.
- wiml 13y agoThe `indent` tool has been part of the normal BSD distro for decades, I think.
- jcurbo 13y agoFunny thing is, I was finally looking at Rust the other day and thought, hey, that's nice that if statements are forced to have braces, I never liked one-liner if statements in C. And here we have a perfect example of what can happen with the brace-less if statements.
- Jayschwa 13y agoYeah, it seems like this is something many newer languages are requiring, which is good. Go requires braces as well.
- lmm 13y agoRequiring braces is the wrong solution to the problem. It's the indentation that misleads here; indentation is what people look at to see what's part of which if, so have the language look at the same thing, like Python does.
- emmelaich 13y agoIndeed, but I wonder if indenting would have helped here. Two gotos immediately after one another at the same indentation level is obviously wrong by visual inspection. Proper indentation might have led the casual viewer to think that the code was actually ok!
- micro-ram 13y agoI use Astyle to enforce both braces and indentation automatically. http://astyle.sourceforge.net/ http://astyle.sourceforge.net/
- rwallace 13y agoThat's the thing - with an indentation-based syntax, the code would have been okay! The second goto would have been part of the if statement, so it would have had no effect beyond the aesthetic.
- dhimes 13y agoI came here to say this. Yes, testing is important. Don't ignore it. But let's not lose sight of the high value of smart, time-tested coding conventions. They exist because of the long, bloody history of mistakes by very smart people.
- kps 13y agoBraces don't help in coding styles that put the opening brace on a separate line from the control statement. if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0) { goto fail; } { goto fail; }
- bunderbunder 13y agoIf you have a programmer on your team who is likely to modify if (condition) { doSomething(); } into if (condition) { doSomething(); } { doAnotherThing(); } instead of if (condition) { doSomething(); doAnotherThing(); } then that person needs some serious mentoring right away. 'Cuz. . . just wow. As far as the original example goes, if it's an error it's most likely a copy/paste error. Curly braces help there, too. With three times as many lines in the block, the odds of a paste error resulting in code that even compiles is greatly reduced, and a compiler error should call attention to the issue. Even assuming the bug was malicious, the curly braces would increase the odds of it being caught by another developer. This is particularly the case now that offside rule languages are common. A large chunk of younger devs cut their teeth on languages where if (condition) doSomething(); doAnotherThing(); doesn't look odd in the slightest. But I think that the 2nd example above would still look immediately bizarre to nearly everyone.
- orangecat 13y agoAs far as the original example goes, if it's an error it's most likely a copy/paste error. Right, and this demonstrates the major problem with verbosity in languages and APIs and design patterns. When you have to repeat yourself many times, it's very easy to make a mistake in one of the near-copies, and you or a code reviewer can miss it because you'll tend to quickly skim over the boilerplate. For cases like this, using exceptions rather than manual error value checking would make your code shorter, less redundant, and not susceptible to this particular bug. (Not possible in C, I know).
- bunderbunder 13y agoNot possible in C, and I'm not even certain that high-level exceptions are desirable in a language like C. But I wonder if there's still room to tighten up the code. Perhaps something like if ( (err = SSLHashSHA1.update(&hashCtx, &serverRandom) != 0) || (err = SSLHashSHA1.update(&hashCtx, &signedParams) != 0) || (err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0)) { goto fail; }