Skip to main content

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;
}
}
else
{
days -= 365;
year += 1;
}
}


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

Comments

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.

Popular posts from this blog

CARDIAC: The Cardboard Computer

I am just so excited about this.


CARDIAC. The Cardboard Computer. How cool is that? This piece of history is amazing and better than that: it is extremely accessible. This fantastic design was built in 1969 by David Hagelbarger at Bell Labs to explain what computers were to those who would otherwise have no exposure to them. Miraculously, the CARDIAC (CARDboard Interactive Aid to Computation) was able to actually function as a slow and rudimentary computer. 
One of the most fascinating aspects of this gem is that at the time of its publication the scope it was able to demonstrate was actually useful in explaining what a computer was. Could you imagine trying to explain computers today with anything close to the CARDIAC?

It had 100 memory locations and only ten instructions. The memory held signed 3-digit numbers (-999 through 999) and instructions could be encoded such that the first digit was the instruction and the second two digits were the address of memory to operate on. The only re…

How To Care If BSD, MIT, or GPL Licenses Are Used

The two recent posts about some individuals' choice of GPL versus others' preference for BSD and MIT style licensing has caused a lot of debate and response. I've seen everything as an interesting combination of very important topics being taken far too seriously and far too personally. All involved need to take a few steps back.

For the uninitiated and as a clarifier for the initiated, we're dealing with (basically) three categories of licensing when someone releases software (and/or its code):
Closed Source. Easiest to explain, because you just get nothing.GPL. If you get the software, you get the source code, you get to change it, and anything you combine it with must be under the same terms.MIT and BSD. If you get the software, you might get the source code, you get to change it, and you have no obligations about anything else you combine it with.The situation gets stickier when we look at those combinations and the transitions between them.

Use GPL code with Closed S…

Using a React Context as a Dispatch Replacement

React Contexts are the pretty little bows of the React world.

Here's a really quick example of the kind of messy code you can cleanup by using contexts, without dragging in a larger dependency like Redux or even Flux. Starting backwards with a diff showing lines of code I was able to remove:


All the properties I was able to remove were just pass-through. The Carousel component didn't care about any of them, but it had to pass through these callbacks so the multiple TaskList components inside the carousel could invoke actions. They were removed from the Component class itself, too, since it no longer needed to pass them through.

Where did they all go?


My ActionContext removed all the need for these passthroughs by providing a single simple helper method, action(), that components rendered under it can access.


I really enjoy the pattern of passing a single callback through a context and removing what used to be lots of callback properties. Of course, I could be using a proper di…