Ship It! - 3.4 Review All Code

Can be painless if:

• Only review a small amount of code.
• There are one or two reviewers at most.
• Review very frequently, often several times a day.

To avoid MAD (Mighty Awful and Dreaded) reviews, separate your work into the smallest possible pieces and get each one reviewed independently and committed into the source code repository. Then if there’s a problem with any one area, it’s easily isolated. A good rule of thumb, however, is to never work for more than two days without getting a code review. Ideally there will be one review for each feature you add (or for each bug you fix).

Tips:

• Code reviews must involve at least one other developer. 
• Do not make code publicly available without a review. Don’t add your changes to the source code from which your product is built until a review has been done. Part of the comments you include with the code’s check-in should list your reviewer’s name. Then, if there are questions about the reason for the code change and you’re not around, there is a second person who should be able to explain it (at least at a basic level).
• Never use this code review rule as an excuse to not check in your code. If your company has a source code system that holds only production code, then put your code into a private area until it’s ready. This private area might be a separate code repository or another installation of the source code management system. Use whatever tools you need to get the code off your machine so that when your machine dies (as machines are prone to do), your code doesn’t die with it.
• Reviewers maintain the right to reject code that they find unacceptable.  
• If your code can’t be explained at a level the reviewer can understand, then the code must be simplified. As a reviewer, don’t sign off on anything you don’t understand and feel comfortable with. After all, your name is associated with this code as the reviewer.
• Any code changes that are made can’t break any existing automated tests. Don’t waste your co-worker’s time asking for a code review if you haven’t yet run the tests. If you require existing tests to be updated, make the changes to those tests a part of the coding before the review. Any new tests that you are adding should be a part of the review as well. As a reviewer, always reject code changes if you think more tests are necessary.
• Rotate the reviewers you use, but don’t be religious about it.
• Keep code reviews informal.
• When you introduce the code review process, you may need to appoint a few senior team members to be the mandatory reviewers; one of the senior team members must participate in every review at first.

Tip 17: It's okay to say "later"

Your management must require code reviews. If there’s no management buy-in, no one in your shop has any official motivation to participate. In other words, if no one has been told to help you, they probably won’t make time to do it, especially when deadlines are tight.

Tip 18: Always review all code

How to Get Started:

• Be sure everyone understands the type of code review you have planned. Review frequently on smaller blocks of code. Don’t wait for weeks, accumulating hundreds or thousands of lines of changes. No MAD reviews for your team!
• Have one of your senior team members sit in on each code review for the first few weeks or months. This is a great way to share knowledge and get the reviews on a solid foundation.
• Make sure your code reviews are lightweight. It’s better to review too little code than too much. Having two overlapping reviews is better than having one larger one.
• Introduce a code change notification system  at this time. It’s a great complement to your code reviews, and it helps to remind team members who forget to ask for reviews.
• Make sure you have management buy-in before requiring all team members to participate. 

You're Doing It Right If...

• Do code reviews get an automatic approval? This shouldn’t happen unless everyone on the team is perfect.
• Does every code review have major rewrites? If so, it indicates a problem somewhere: either with the coder, with the reviewer, or with the tech lead (who gave the directions that the coder and reviewer are using).
• Do code reviews happen frequently? If the time between reviews is measured in weeks, you’re waiting too long.
• Are you rotating reviewers?
• Are you learning from the code reviews? If not, start asking more questions during your code reviews.

Wow.  I'm thinking code reviews is an area where there will be much resistance.  Only one of my past shops did code reviews and I can't even remember if it helped quality.  I do remember, however, people getting pissed because they couldn't check in their code in time for a build due to a failed review.  They got angry because the coder disagreed with the reviewer that the code should not be allowed to go in over a "minor" issue.  I'm wondering if automation can be of any help here?  There are tools that can check for silly code style violations, like where the curly braces go, which might lessen the reviewer's load.  There are also tools like FindBugs that can detect threading issues which, again, might lighten the reviewer's load.

One thing I'm wondering about is how to avoid checking in code without a review, especially when working with remote developers?  Do I keep my own branch that gets merged with the trunk every day?  I could check in my proposed change into the branch, have it reviewed and then merge it into the trunk if it passes inspection.  E-mail the files? Are there other techniques?