Code Review Options

There are many ways to conduct a code review.  Here are a few ways I've seen it done and the advantages and disadvantages of each.

Over-the-Shoulder Reviews

Walk over to someone's office or invite them to yours and walk them through the code.  This has the advantage that the author can talk the reviewer through the code and answer questions.  It has the disadvantage that it pressures the reviewer to hurry.  He won't want to admit he doesn't understand and will sign off on something without really validating it.  It also doesn't work for teams that are not co-located.

E-mail Based Reviews

This model requires some method of packaging the changes.  This is then e-mailed to another person for review.  The reviewer than then open the code in WinDiff or a similar tool and review just the deltas.  This tends to be efficient and allows the reviewer ample time to read and understand the code.  The downside is that the reviewer can take fail to prioritize the review correctly and it can sit around for a long time.

Centralized Reviews

Code reviews are sent (via an e-mail based system usually) to a centralized team of trained code reviewers.  These are usually the senior coders.  It has the advantage of helping to maintain high quality and uniformity in the reviews.  In a team trying to change coding styles or increase the quality of their code, this can be  a great way to do so.  The downside is that this team becomes a bottleneck.  If only a small portion of the team must review all the code, they can fall behind and reviews can back up.  Being part of the central review team can also be a drain on the most efficient coders.  Instead of writing code, they spend a large portion of their time reviewing code.

Decentralized Reviews

In this format, everyone in the team participates.  Any person can review any other person's code.  This form scales most easily with team size but can require some tools if it is to work on a large team.  Otherwise it becomes too hard to tell which people are participating and which are not.  The advantage of this form is that everyone is exposed to both sides.  This allows junior members to learn by reading code written by more experienced members.  The disadvantage is that the quality of the reviews is uneven.  Different members will have different abilities.  Over time this should even out as the team grows together.

Group Reviews

So far I've been speaking of reviews which involve only two people.  The reviewer and the reviewee.  There is another model in which a group gathers together in a room and the reviewer leads them through the code.  This model puts a lot of eyeballs on the code and is likely to find the issues better than any individual reviewer, but it also costs a lot of eyeballs.  This model uses the most manpower and is the least efficient reviewing model.  It suffers the same problems as the over-the-shoulder reviews except exacerbated.  The likelihood of people not being able to follow is high.  Additionally, the review will likely go slowly as people bring up questions about various points.  Finally, it may bog down into argument between the reviewers.  This model can be improved if participants review the code ahead of time and come prepared to only talk about their issues.  If they reviewers aren't reading the code in the meeting, it can be efficient.  Sometimes this is called a code inspection.

What We Use

There is no single right way to conduct reviews.  I won't pretend to proscribe a single manner of conducting them that will fit all organizations.  I will, however, tell you what I've found to work.  My team uses a distributed e-mail model.  We have a tool which will take the diff of the latest code in the repository and the code on your box and package it up.  The reviewee then sends mail to a mailing list asking for someone to review his or her code.  A reviewer volunteers and responds letting everyone know he is taking the review.  The reviewer then reads the code, sends his comments to the reviewee.  The reviewee then makes any required changes and eventually checks in the code.

Code Review Rights and Responsibilities

Code reviews are an important part of any project's health.  They are able to find and correct code defects before making it into the product and thus spare everyone the pain of having to find them and take them out.  They are also relatively cheap.  Assuming your team wants to implement code reviews, it is important to lay out the expectations clearly.  Code reviews can be contentious without some guidelines.  What follows are the rights and responsibilities as I see them in the code review process.

Participants:

Reviewer - Person (or persons) reviewing the code.  It is their job to read the code in question and provide commentary on it.

Reviewee - Person whose code is being reviewed.  They are responsible for responding to the review and making necessary corrections.

Must Fix Issues:

A reviewer may comment on many aspects of the code under review.  Some of the comments will require the reviewee to change his code.  Others will be merely recommendations.  Must fix issues are:

• Bugs - The code doesn't do what it was intended to do.  It will crash, leak memory, act erroneously, etc.
• Potential maintenance issues - The code is not broken, but is written in such a way that it will be hard to maintain.  Examples might be magic numbers, poorly named variables, lack of indirection, lack of appropriate comments, etc.
• Coding standard violations - If the group has a coding standard, it must be followed.  Deviations from it must be fixed when pointed out.

Recommendations:

Other items are merely recommendations.  The reviewer can comment on them, but the comments are only advisory.  The reviewee is under no obligation to fix them.  These include:

• Architectural recommendations - The reviewer thinks there is a better way to accomplish the goal.  Seriously consider changing these, but the reviewee can say no if he disagrees.
• Style issues - The reviewer wouldn't have done it that way.  Fascinating.

Code Ownership:

In my teams there is no ownership of code.  Some people touch certain pieces of code most often and may even have written the code initially.  That doesn't give them special rights to the code.  The person changing the code now does not need to get the initial writer's permission to make a change.  He would be a fool not to consult the author or current maintainer because they will have insights that will help make the fix easier/better, but he is under no obligation to act upon their advice. 

The Company You Work For Matters

In an episode of Stack Overflow Podcast, Jeff Atwood made an interesting point.  He spoke of the difference between companies that deliver code as their product (like Microsoft) and companies who use code to support other business interests (the IT department inside Weyerhauser for example).  In a company whose business model is to make and sell software, making the software incrementally better supports the business function and is thus rewarded.  In an IT department, once the project crosses a certain threshold, the returns on making it better diminish rapidly.  Improving the code is often discouraged.  All this has been said before.  Joel Spolsky has talked on this subject as have I. 

What Jeff added to this conversation was a practical application.  He rightly pointed out that you need to be aware of the type of person you are and the type of company you are working for.  If you love to code, you really don't want to work in an IT department.  Sure, someone will pay you to code, but you'll be frustrated by the limitations on your work.  If you enjoy programming, you want to work for a company whose job it is to produce software.  On the other hand, if you are willing to program but it's just a job, you don't want to work in a software house.  An IT department will be a better fit and you will find your career better served by working there.  In an software house, you'll be asked to give more of yourself and be more creative than you might otherwise like to.

Test Code Must Be As Solid As Dev Code

All good development projects follow certain basic practices to ensure code quality.  They use source control, get code reviewed, build daily, etc.  Unfortunately, sometimes even when the shipping product follows these practices, the test team doesn't.  This is true even here at Microsoft.  It shouldn't be the case, however.  Test code needs to be just as good as dev code.

First flaky test code can make determining the source of an error difficult.  Tests that cannot be trusted make it hard to convince developers to fix issues.  No one wants to believe there is a bug in their code so a flaky test becomes an easy scapegoat.

Second, spurious failures take time to triage.  Test cases that fall over because they are unstable will take a lot of time to maintain.  This is time that cannot be spent writing new test automation or testing new corners of the product.

Finally, poorly written test code will hide bugs in the product.  If test code crashes, any bugs  in the product after that point will be missed.  Similarly, poor test code may not execute as expected..  I've seen test code that returns with a pass too early and doesn't execute much of the intended test case.

To ensure that test code is high quality, it is important to follow similar procedures to what development follows (or should be following) when checking in their code.  This includes getting all non-trivial changes code reviewed, putting all changes in source control, making test code part of the daily build (you do have a daily build for your product don't you?), and using static verification tools like PCLint, high warning levels, or the code analysis built into Visual Studio.

Cause of "Internet Explorer cannot open the Internet Site" found

Perhaps you, like me saw a rash of sites giving the error "Internet explorer cannot open the Internet Site" yesterday.  At first I thought one of my machines was messed up.  Then I saw it on another machine.  This one running XP.  A virus perhaps?  I can't recall doing the same thing on both machines.  How would I get a virus on both?  A worm maybe?  Updated the antivirus just in case.  This morning the errors seemed to be gone but still the mystery.  What caused it?

It appears that a tool called SiteMeter was causing the problem.  Mystery solved.  More detailed information here.