Thursday, January 01, 2009

How To Prove Code Review is Important

The infamous Zune 30GB failures were traced to a leapyear issue, and apparently they use some code we can see in the Freescale codebase. Take a look at the following sample of code, which determines the year from the day number (counting from January 1, 1980). I don't know about you, but the infinite loop is immediately obvious. On a leapyear, the main loop continues when days = 366 and the incrementing is never reached, because days > 366 fails.

Am I naive to think that even a casual code review would have caught this in a moment?

year = ORIGINYEAR; /* = 1980 */
while (days > 365) {
if (IsLeapYear(year))
if (days > 366)
days -= 366;
year += 1;
days -= 365;
year += 1;

UPDATE: Fixed formatting issues. It looked fine when I posted it, honest!


fumanchu said...

My best guess: the original author missed it because the 'else' block was indented to the same level as the inner 'if' at some point. I'd give good odds a reviewer would have missed it too.

Now a test--that would have caught it easily. Or a language without curly braces.

David Grant said...

How about adding some carriage returns so I can read it.

I write here about programming, how to program better, things I think are neat and are related to programming. I might write other things at my personal website.

I am happily employed by the excellent Caktus Group, located in beautiful and friendly Carrboro, NC, where I work with Python, Django, and Javascript.

Blog Archive