Hacker News Reader

Comment by badman_ting | Hacker News Reader

If blocks without curly braces ಠ_ಠ

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

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

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

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

Good examples actually. (Makes me feel better about my ingrained semicolon habit.)

Your examples will crash immediately and at the right spot though. The problems I see caused by excessive use of semicolons are often far weirder.

That said, inadvertent errors caused by semicolon insertion are still more common and baffling (especially by people addicted to jslint who use a variable declaration idiom particularly easy to screw up with errant semicolons).

The first example won't crash if the rvalue preceding the IIFE is a higher-order function (specifically, one that outputs a function).

A relatively rare scenario, but a brutal one to debug.

As for errors caused by extra semicolons, they can be weird, but I don't think I've ever actually hit one in practice. They'd also be a little easier to spot, since you tend to develop a reasonable instinct for where semicolons belong.

I'd say it's a lot easier to be suspicious of lines of code that start with '(' or '[' than find semicolons at the end of the wrong line. It's incredibly easy to put a semicolon where a comma is expected and vice versa. I do it all the time (usually causing an immediate error so it's not a huge deal).

"It's actually quite difficult and artificial to find cases where (especially with "use strict") semicolons confer any benefit at all,"

Hence these are the cases where more time and resources will be wasted because of it.

"I certainly see far more bugs caused by an improperly inserted semicolon"

What would be an example of this? Because I've seen exactly zero bugs of this type (not counting typos, of course)

var a = 17, b = 13; c = 5;

(usually this will be across multiple lines)

...just overwrote c in a different scope. This kind of bug is common, idiomatic, baffling, and actually more likely among coders subscribing to javascript "best practices".

use strict? jshint / jslint your code?

This doesn't justify playing a guessing game and skipping semicolons just because you think you know all the rules about not using them.

jslint won't stop it if there's a variable (c in this case) in the outer scope -- it's perfectly fine code as far as jslint is concerned. And jslint encourages that declaration style (actually encourages is too weak a word).

I do write single statement conditionals without braces, but I write them on one line, which emphasizes the single-statement nature:

  if (something) do_something();

Instead of:

  if (something)
      do_something();

So it's much less likely that I'll confuse indentation with a block scope.

> I do write single statement conditionals without braces, but I write them on one line

That's fine until you write

    if (something) do_something(); do_something();

or worse:

    if (something); do_something();

I've actually seen something very similar to that one.

You haven't really changed the dimensions of the problem by putting it all one one line. Only the whitespace is different. Sure it looks wrong to you; but so does the incorrect code from apple.

I don't understand the need to omit braces; it doesn't make the compiled program any smaller. And denser source code is not always more readable, or we'd prefer "? :" to "if" every time.

Note that braces won't save you from

    if (something); { do_something(); }

(Personally, I configure my editor to highlight if (...); in bright red.)

This is true; there is no silver bullet. So we rely on defence in depth.

The first line of defence is consistent code layout; e.g. always use "{ }" after an if, and only one statement per line. This aims to make simple typos less likely to compile, make the code more readable and so make errors stand out.

Then there is using and paying attention to the linter / compiler warnings / jshint + 'use strict' / static analysis or whatever it’s called in your language of choice.

Then there are unit tests or integration tests.

Any of these might have caught the apple error, but there are no absolute guarantees.

IMHO it's a professionalism issue – gifted amateurs are great, and they can succeed... most of the time. But a professional engineer would put aside ego "I won’t make mistake like that" and laziness "I don’t need to type braces this time" and do the careful thing every time. Because each step raises the bar, and makes it harder to ship code with bugs like this. Because sometimes it matters.

well you can also write:

if (something); { do_something(); }

So..

your first example is okay, but for me, I'll do this;

if(something) {do_something();}

Many code editors will easily insert curly brace pairs, so it's really just a single extra key stroke you're saving.

Yes, 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.

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

Yeah, it seems like this is something many newer languages are requiring, which is good. Go requires braces as well.

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

Indeed, 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!

That'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.

I use Astyle to enforce both braces and indentation automatically.

http://astyle.sourceforge.net/

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

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

I have emacs setup to auto run clang-format on each save of cc-mode files. I can't NOT notice this stuff now.

The `indent` tool has been part of the normal BSD distro for decades, I think.

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

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

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

That'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".

Braces 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;
  }

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

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

Not 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;
  }

I understand that there are sequence points at those || divisions, and I appreciate that you've been careful with your layout. Even so, my spider sense is tingling horribly at having not just one assign-and-test in the condition for an if statement but a whole series of them that each reassign to the same variable.

If there were a language feature available to do this more elegantly, whether exceptions or something else such as a Haskell-style monad with fail semantics, I'd almost certainly agree with using it in preference to a series of manual checks, though.

Should the variable reuse matter? Is there a case where the compiler won't short-circuit the || statement on the first failure?

If not, `|| (err = check())` is equivalent to separate checks which also also `goto fail` immediately.

>> Is there a case where the compiler won't short-circuit the || statement on the first failure?

Left-to-right, short-circuit evaluation of the logical operators is so deeply rooted in C idiom that a mainstream compiler that doesn't respect it would be practically unusable. But perhaps it wouldn't work in a hobbyist compiler, or a non-standard dialect of C.

Indeed, the code as presented there had well-defined behaviour according to the standard. Note to lunixbochs: This is why I mentioned sequence points before.

While valid according to the language definition, assignments within conditional expressions do have a reputation for being error-prone and not always the easiest code to read. Sometimes they're still neater than the available alternatives.

However, IMHO in this case, it feels a bit too easy to break the code during later maintenance edits. For example, someone might simplify the code by removing the explicit comparisons with 0 that are unnecessary here, and then later someone else might "fix" an "obvious bug" by replacing the "(err = x) || (err = y)" construction with the common construction "(err == x) || (err == y)" that they assumed was intended, perhaps prompted by a compiler warning. Obviously they shouldn't if they didn't properly understand the code, but when has that ever been a reliable guarantee? :-)

That is a lot more obviously wrong that without the braces, though.

But yes, mandatory braces on the same line is the correct choice.

Another solution, (popular at Microsoft FWIW) if you have a common pattern like

    if ((err = DSomething()) != 0)
        goto fail;

is to wrap it in a macro

and goto statements everywhere :S

the gotos actually make sense in this case. unless you'd prefer some insane tree of if/else?

I 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;
  }

I 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; }

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

True, 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.

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

I didn't do a survey but I remember that and frequently punting on error handling (`res, _ = something_which_could_error()`) showing up enough in the projects I saw on Github to stand out as a trend when I was writing a few first programs. I certainly hope that's just sampling error.

The `else if` is useless here, since you return anyway. It only adds noise.

I rarely use the one-line `if err := ...; err !=nil ` idiom because its quite a mouthful. However when I do, I try to make sure it's not too much to grasp at once. Here the extra `else` goes against that.

Alright, I know this is just a quick snippet on HN and all, I just thought I'd mention it anyways. Maybe next time you actually write that in code you'll think about my point. ;)

Couldn'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.

No need for that. Just return early. Plus one should split up this gigantic function into several smaller ones.

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

Reply on news.ycombinator.com