Beware of Optional Curly Braces — They Will Bite You

Posted Monday, January 30th, 2012 at 7:26 pm

I was looking through some PHP code from a third-party vendor recently, and saw something that made my jaw drop. It’s pretty innocent-looking, at first. Here’s a somewhat anonymized and genericized version of the code, but the thing that bothered me is still intact. It’s not really a bug, per se; the code will function as intended. But…

$currentRow = 0;
$itemId = "";
$index = 0;
while ($row = mysql_fetch_object($result)) {
     if ($currentRow == 10) {
          renderHeaderRow();
          $currentRow = 0;
     }
     // takes an itemId and displays relevant columns
     renderSummaryRow($row->itemId);
     $currentRow++;

     if ($index > 0)
          $itemId .= ","; # interpolate a comma
     $itemId .= $row->itemId;
     $index++;
}

See the problem? Really, there are a few ways this can go wrong. To a quick glance, the only clue that line 14 (“interpolate a comma”) is part of a conditional is its indentation. The indentation is important to a human reader — but absolutely irrelevant to the PHP interpreter, which simply treats the next line after the conditional as the conditional’s block. Regardless of how it’s indented, and regardless of what else is around.

The way it looks to a human is not the way it looks to the machine.

What happens if someone wants to add some logging? What if they add it after the comma line?

if ($index > 0)
     $itemId .= ","; # interpolate a comma
     writeLog("Added a comma");
$itemId .= $row->itemId;
$index++;

Now the log claims the code has added a comma, even when it hasn’t. But still, it could be worse! What if you decided to add your logging before the other line?

if ($index > 0)
     writeLog("Adding a comma to itemId...");
     $itemId .= ","; # interpolate a comma
$itemId .= $row->itemId;
$index++;

Now it adds a comma no matter what — even the first time through the loop, when the string is empty. So instead of a string like '123,124,125', $itemId will now have a leading comma: ',123,124,125'. Since this value is getting stitched into a SQL query later on, it means your app will blow up with a SQL syntax error.

This is why Python makes whitespace significant to program flow. The way the indentation makes it look like the logical structure is, is how the structure actually is.

And this is also why Perl — of all languages, one that normally errs on the side of letting you leave out anything that can be inferred from context — Perl insists in its syntax documentation that in cases like this, “the curly brackets are required — no dangling statements allowed.” (It then says, in typically Perlish fashion: “If you want to write conditionals without curly brackets there are several other ways to do it.”)

If you’re working in one of those languages that lets you omit curly braces around a single-statement conditional — DON’T DO IT! The potential maintenance and debugging problems are not worth the fun of saving two keystrokes (or just one, if you work in an editor that auto-closes your braces for you).

One Comment

  1. Posted Tuesday, March 13th, 2012 at 4:03 am | Permalink

    Thanks – as a C/Perl/Assembler/PHP/Java coder, I’d never really thought about Python’s “meaningful whitespace” approach that way before, just thought it weird, but it does make sense from that perspective.

    The curly brackets sometimes seem redundant, but it does help – and even with Javascript, you should really run it through something like Google’s Closure Compiler before deploying, which will strip all that out for you so nothing is really wasted by including them. (I’m de-lousing a third party’s code at the moment – there are entire 200+ line source files which contain no comments whatsoever. Or rather, in this particular one, 193 lines, two of which are commented-out code.)

Post a Comment

Your email is never shared. Required fields are marked *

*
*