The topic of this post sorts reminds me of the whole DbC mess that paralyzed our group for years. Wow, the fallout from some key bad decsions! Well, this example is much more harmless, but I thought it was interesting.

A tester discovered that the SQM code wasn’t achieving the correct code coverage numbers. The target is normally 85% and the SQM code was hovering somewhere around 60%. Why was this?

Because there was a lot of code that looked like this:

if( FAILED( sqmInitStatus_ ) )
{
   HWSW::Trace( L"SQMSession::Initialize:: SQM temporary file path error: %s", pSqmFileLocation );
   return sqmInitStatus_;
}

Normally, getting inside an “if” statement like this is quite easy from a unit test. You simply have to write a test that forces that error condition. Then, testing your error handler is a simple matter. The only problem was that the author used a traditional method for validating string arguments at the beginning of functions. These traditional validation methods are fine for string validation when you’re using functions like strcat and strcpy. But, if you’re using the safe string functions (StringCch*), then is it overkill?

 if ( pAppVersion == NULL ||
 ( ::wcslen( pAppVersion ) == 0 ) || 
 ( ::wcslen( pAppVersion ) > MAX_VERSION ) ||

…

If these any of these conditions fail, the function will return an INVALID_PARAMETER return value. It’s therefore basically impossible for the StringCch* functions to fail, which makes getting inside those if statements impossible.

So, what should be done?

My opinion is to modernize this sort of code. The code does not need so much argument validation at the beginning because the StringCch* functions manage a lot of this. Also, writing unit tests early could have helped catch this sort of problem.

History:

After some time, I started getting the idea that the evolved code review process may not be so good. Basically, team members use SD Pack and email. Everyone on the team gets the email and everyone sends the review back to the author. The author then pastes all the responses back into another email, comments on each response, and then sends the email. It suffices to say: Not a great use of time (the author has to do a lot of reading, synchronizing email directions with real code), and encourages superficial comments (everyone spends the same amount of time on general areas).

Action:

I started brainstorming ways to improve code reviews. At first I was just thinking of having more sit down paper code reviews. Fortunately, and very serendipitously, an EE training class on code reviews came to Shenzhen. It was taught by a principle developer on loan here in China from Redmond. He’s a solid guy, awesome dev, and one of the best teachers I’ve seen here.

The training was exactly what we were looking for. The great thing was that it actually demonstrated an easy way to measure how useful your code reviews are. We brainstormed some more on this topic, I gave three presentations to the team on the various ideas, we had one quick sample code inspection, and have derived a streamlined process that we’re trying out here at MACH.

My goal: Make the process easy and rewarding.

Process:

The training focused mostly on “code inspections”. We’ve taken some of the ideas and simplified it:

For example, code inspections identifies roles like: Gatekeeper, Moderator, Scribe. We’ve rolled many of these together. We still see value in a moderator (someone other than the author who runs the meeting), but that person can also act as the scribe (writing down bug statistics). The Gatekeeper (the one who ensures the code is ready to be reviewed) can be performed by each author (using an optimized and more visible version of the style guide, there should be no style issues).

There are simple but effective statistics that we will take too:

1. Counting the total number of issues.

2. Of the total issues, which ones are major? Major issues should be labeled and counted. Right now, we’re defining “major” as: Any issue that causes a crash, leaks a resource, hangs the application, affects performance, serious thread issues, or maintainability and design problems. If we know our team’s bug fix rate (we are calculating this now), then we can calculate if the code review was a good use of time. Also, as the percentage of major issues rise (without the overall of quantity of Major issues rising), we’re getting better at Gatekeeping and style.

3. Of the total issues, which ones overlap? Meaning, how many people found the same issue? This helps us to judge the effectiveness of the review. If similar issues, especially major issues, are found, the we have higher confidence in the effectiveness of the code review at finding bugs.

There are two other documents that fell out of this exercise:

1. Improved Style Guide: This idea came from the realization that most of our comments were high level style issues. So, how do we fix it? The first thought was to have a more useful style guide. So, I took the initiative to cut 80% out of the style guide so that it only covered style (not coding best practices or project settings)! We want to refer to it frequently and make changes as needed. One of the main goals though is to keep it short.

2. Code Review Checklist: This idea came from the training class. The team brainstormed and created this list. We haven’t made much use of this yet, but the idea is it’s a reminder of what’s important to review and, more importantly, if we need someone to focus deeply in a particular area, the checklist helps with this.

Of course, this process would not be not required for all code reviews, but only for the biggest, most important reviews. Walk-in peer reviews are still useful for quick reviews, SD Pack and email reviews are still useful in small sections of existing code that you’ve already touched, etc. Part of my job here will be to help the team members understand just when the appropriate review is necessary.

LINQ is basically a translation layer between the class paradigm for programming languages and rows in a relational database. Instead of the programming languages using SQL to query the database, they use instances of their classes and LINQ together. LINQ does all the SQL interaction.

When I first started reading about it, it reminded me of “Ruby on Rails”, specifically the “Convention over Configuration” paradigm.

For example, look at this C# class:

[Table(Name="Customers")]
public class Customer
{
public string CustomerID;
public string City;
}

And now this quote from the MSDN help: “The Table attribute has a Name property that you can use to specify the exact name of the database table. If no Name property is supplied, LINQ to SQL will assume the database table has the same name as the class.”

Except, there’s one little problem. Does the class really need to have the same name as the Table, or does the class need to have the same name as the Table minus the “s”?

Hopefully, I’ll remember to update this post when I learn the answer.

Oh, but look at this code!

[Table(Name="Customers")]
 public class Customer
 {
 [Column(IsPrimaryKey=true)]
 public string CustomerID;
 [Column]
 public string City;
 }

Now, there’s a “Column” keyword that tags the data member as a column in a database table. According to the same MSDN Help, “As with the Table attribute, you only need to supply information in the Column attribute if it differs from what can be deduced from your field or property declaration.”

This is “Convention over Configuration” at work.

If I were to answer the question, “What are the elements of a worthwhile code review?”, to a IT reporter in need, I would answer something like this:

A proper code review is a valuable tool to any software team. The longer that bugs go undiscovered, the more costly it is to fix them. For example, if a developer works out a design on a whiteboard, updates and improvements can be made very cheaply, just by erasing part of the software diagram and starting again. After the product has shipped, an undiscovered bug becomes very expensive to fix, usually through a laborious patching process. A code review is a way to reduce costs by eliminating bugs early. Code reviews occur early enough to catch bugs before they have an opportunity to become very expensive to fix.

But, in order for the code review to be useful, it should be conducted in a responsible manner by a mature team with a good leader. All developers have certainly experienced code review horror stories or at least ones that qualify as wastes of time: Reviewers come to the meeting unprepared and begin reading the code line-by-line, or, reviewers focus on silly issues like “oh, this ‘for’ statement should have a space after the opening parenthesis.”

There are ways to eliminate these sorts of time-waster. There are even ways to measure if your code reviews are worth the time spent. After all, if you’re spending more time in your code review than you could have been fixing bugs, then your code reviews are not useful. Of course, this is rare.

First of all, to eliminate the silly code review comments like spaces or capitalization, you need a good style guide. A style guide describes how the team prefers to comment, how the team prefers to name classes, variables, and methods, and how to format. It should not attempt to convey software best practices, like how best to deal with memory that can’t be allocated, or tracking threads, and so on. It should only focus on style. The style guide should be short enough so that it can be easily grasped by everyone on the team and not cover every nauseating detail so that team members still have a sense of freedom to write the code the way they see fit. After all, writing code can be very personal.

Developers are then held accountable that they will have no style issues in their code. If the team has established good style guidelines which are readily accessible and used frequently, this ought to be easy.

To ensure that everyone prepares in advance, you can create some fun rules. For example, we have simple rule that says, “If you have no red marks on your code print-out’s you’re not allowed into the review. Red marks are the ticket!”

To help the team focus on important issues, a code review checklist is useful. The team can create this together, again keeping it very short so that it’s easy to refer to, and then use it to coordinate focusing on specific areas. Too often, poorly run code reviews has everyone focus on the same superficial issues. A code review checklist can help encourage a smaller group to focus deeply on a specific area, another group to focus on a different area, and so on. This helps the code review achieve depth.

It’s also a good idea to have someone other than the author to run the code review meeting. Software developers tend to be a proud lot and an independent moderator can keep the discussion from getting too personal. A good moderator is someone good at running meetings, keeping people on track and focused on important issues.

Finally, you can calculate just how effective your code reviews are by having two pieces of information: The average bug fix rate for a team member (usually ranges between 1 and 2 per day), and how many major issues you uncovered in your code review. Assume you found five major issues in your last code review and your team’s major bug fix rate is one per day. If there was no code review, it would have cost approximately five days to fix those bugs (the number of bugs times how long it takes to fix a major issue). If your code review had five participants, each prepared for one hour, and the review meeting lasted one hour, the total time is 10 hours. That’s well less than five days, so your code review was worthwhile!

Our team in China is just now adopting these changes and we’re excited about it. We especially enjoy being able to measure the effectiveness our our reviews.