CodeSOD: The Sound of GOTO

Let's say you have an audio file, or at least, something you suspect is an audio file. You want to parse through the file, checking the headers and importing the data. If the file is invalid, though, you want to raise an error. Let's further say that you're using a language like C++, which has structured exception handling.

Now, pretend you don't know how to actually use structured exception handling. How do you handle errors?

Adam's co-worker has a solution.

 char id[5]; // four bytes to hold 'RIFF' bool ok = false; id[sizeof(id) - 1] = 0; do { size_t nread = fread(id, 4, 1, m_sndFile); // read in first four bytes if (nread != 1) { break; } if (strcmp(id, "RIFF")) { break; } // ... // 108 more lines of file parsing code like this // ... ok = true; } while (time(0L) == 0); // later if (ok) { //pass the parsed data back } else { //return an error code }

This code was written by someone who really wanted to use goto but knew it'd never pass code review. So they reinvented it. Our loop is a do/while with a condition which will almost certainly be false- time(0L) == 0). Unless this code is run exactly at midnight on January 1st, 1970 (or on a computer with a badly configured clock), that condition will always be false. Why not while(false)? Presumably that would have been too obvious.

Also, and I know this is petty relative to everything else going on, the time function returns a time_t struct, and accepts a pointer to a time_t struct, which it can initialize. If you just want the return value, you pass in a NULL- which is technically what they're doing by passing 0L, but that's a cryptic way of doing it.

Inside the loop, we have our cobb-jobbed goto implementation. If we fail to read 4 bytes at the start, break to the end of the loop. If we fail to read "RIFF" at the start, break to the end of the loop. Finally, after we've loaded the entire file, we set ok to true. This allows the code that runs after the loop to know if we parsed a file or not. Of course, we don't know why it failed, but how is that ever going to be useful? It failed, and that's good enough for us.

This line: char id[5]; // four bytes to hold 'RIFF' also gives me a chuckle, because at first glance, it seems like the comment is wrong- we allocate 5 bytes to hold "RIFF". Of course, a moment later, id[sizeof(id) - 1] = 0; null-terminates the string, which lets us use strcmp for comparisons.

Which just goes to show, TRWTF is C-style strings.

In any case, we don't know why this code was written this way. At a guess, the original developer probably did know about structured exception handling, muttered something about overhead and performance, and then went ahead on and reinvented the goto, badly.

[Advertisement] Continuously monitor your servers for configuration changes, and report when there's configuration drift. Get started with Otter today!