The Zunes Freezes: Update!

The faulty code was leaked some time last week, and I’ve looked at it, and, well, it’s surprisingly clean (despite the obvious defect and how they should’ve found it before releasing it (see here)).

Have a look at the ConvertDays routine.

//------------------------------------------------------------------------------
//
// Function: ConvertDays
//
// Local helper function that split total days since Jan 1, ORIGINYEAR into 
// year, month and day
//
// Parameters:
//
// Returns:
//      Returns TRUE if successful, otherwise returns FALSE.
//
//------------------------------------------------------------------------------
BOOL ConvertDays(UINT32 days, SYSTEMTIME* lpTime)
{
    int dayofweek, month, year;
    UINT8 *month_tab;

    //Calculate current day of the week
    dayofweek = GetDayOfWeek(days);

    year = ORIGINYEAR;

    while (days > 365)
    {
        if (IsLeapYear(year))
        {
            if (days > 366)
            {
                days -= 366;
                year += 1;
            }
        }
        else
        {
            days -= 365;
            year += 1;
        }
    }


    // Determine whether it is a leap year
    month_tab = (UINT8 *)((IsLeapYear(year))? monthtable_leap : monthtable);

    for (month=0; month<12; month++)
    {
        if (days <= month_tab&#91;month&#93;)
            break;
        days -= month_tab&#91;month&#93;;
    }
    
    month += 1;
    
    lpTime->wDay = days;
    lpTime->wDayOfWeek = dayofweek;
    lpTime->wMonth = month;
    lpTime->wYear = year;

    return TRUE;
}

Note that the code makes careful use of a non standard series of #define or typedefs, like UINT8 and UINT32, uses System Hungarian (we’ll talk about this soon, in an upcoming entry), and is generally very readable.

So, clearly, code quality was intented, how come they did not trap the error? As I said earlier, someone obviously forgot to test his code thoroughly—and deserves a spanking.

However, there are a few elements of style I do not like. But that’s entirely personnal, but I think I can rationalize part of it. Consider the first code excerpt:

UINT8 *month_tab;
...
// Determine whether it is a leap year
month_tab = (UINT8 *)((IsLeapYear(year))? monthtable_leap : monthtable);

can be made a lot clearer by a fusion of tables:

month_tab=month_table[IsLeapYear(year)];

where month_table is defined as UINT8 month_table[2][12]={{...},{...}}. The expression month_table[0] is already a UINT8*.

The other objection targets break and continue, two long time pet peeves of mine. Using break, well, breaks the flow of code. The following loop:

for (month=0; month<12; month++)
 {
  if (days <= month_tab&#91;month&#93;)
     break;
  days -= month_tab&#91;month&#93;;
 }
&#91;/sourcecode&#93;<br><br>

could have been made into:<br><br>


for (month=0; days>months[month]; days-=months[month],month++);

or even:

month=0;
while (days>months[month])
 {
  days -= months[month];
  month++;
 }

but I would not use the syntactically correct but much stranger:

month=0;
while (days>months[month])
    days -= months[month],
    month++;

In either case, I made sure they computed the same thing using a simple unit test:

int leap;
for (leap=0;leap<2;leap++) { int day; for (day=0;day<365+leap;day++) { int zune_month=zune_convert_month(day,monthtable[leap]); int month=convert_month(day,monthtable[leap]); if (zune_month!=month) printf("fail on day: %d, zune=%d, new=%d\n",day,zune_month,month); } } [/sourcecode] Of course, it’s not like it’s grand art, but it covers the two cases: leap years and non-leap years, assuming days are numbered from 0 to 364 or 365 on leap years.

3 Responses to The Zunes Freezes: Update!

  1. mathmoi says:

    Hi,

    Clean or not, I think there is another bug in the posted code. At the end of the function at line 143 there’s a call to what looks like a logging function. This function is called a 3 other places in the code (lines 609, 726 and 767), but there is something wrong with the code at line 143.

    Just before it, at line 140 there is an empty else block (empty as in no statement and no empty block). In this case, the next statement, our call at line 143, becomes the code of the else part of the condition and is only executed if the condition is false.

    I don’t think this is intended. The OALMSG(…) function looks like it’s a logging function that must be called at the end of the function. At least it’s what happens in the 3 others places it’s called in the posted code.

    MP

  2. […] and the company. Remember, more than year ago I told you about how avoidable was the Zune bug, why and how the bug should have been found, and how sometimes even simple testing strategies can help you get […]

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s

%d bloggers like this: