Sep 10 2008

Peer Reviews

Category: Best PracticesMatt @ 09:35

Peer reviews seem to be a source of seething hatred for many developers.  I can sort of understand why (no one likes rocks being pitched at something they created), but they can actually be very beneficial if done correctly.  This post will lay out the case for doing peer reviews as well as an approach to performing reviews that I happen to like (stolen from my friend, 'Poprythm').  If you have any suggestions for ways to improve this approach, please share in the comments!  Also, if you're not doing peer reviews, I'd like to hear why.

Purpose

Peer reviews may seem like a hassle, but they actually serve several purposes. First, they help identify and address deficiencies in the code that will become maintenance issues later. By finding them early (or at least earlier), the cost of fixing them is somewhat reduced. Identifying some deficiencies, such as code that is completely unreadable or that is unnecessarily complex, will help save time and money (and also sanity) down the line.

Another benefit of code reviews is that they can help spread knowledge. Both the person doing the review and the original author can learn new things via the process. The original author gets feedback on ways to improve their code, such as alternative algorithms, syntax shortcuts, and design improvements. The person doing the review also gets several of those same benefits (the author may have solved a problem elegantly in a way that the reviewer had never thought of), but they also gain experience in reading code. The ability to read and understand code, especially someone elses, is an underrated skill. Becoming proficient at it may help you to analyze and troubleshoot problems more easily in the future.

Performing the review

When performing the review, follow a few simple rules. First, do not make any code changes at all. When you spot something that needs to be addressed, place a comment in the code prefixed with "PEER:", and explain the issue with the following code block. For example:

   1: //PEER: This will cause a run-time exception!
   2: if (value == null)
   3: {
   4:    logger.Log("Found a null value: " + value.Name);
   5: }

(Sadly yes, that is a real example I ran into from many years ago...)

Second, while no one likes a critic, it is important to identify any and all issues. If the author used a variable name that doesn't make since (such as using 'l' for a logger instead of a more descriptive name), go ahead and note it. It may seem trivial on its own, but trivial issues add up.

Third, (obviously) try to be constructive with any criticism. If you identify a problem, try to explain why it is a problem and offer a couple of ways to address it.

Peer reviews are somewhat subjective, and everyone tends to look for slightly different things. However, here are the things you should absolutely be looking for:

  1. Failure to adhere to coding guidelines or best practices.
  2. Untested code. TestDriven.NET includes a code coverage analysis tool, so use it to examine the code in question. Note any areas that are not tested. While high test coverage doesn't necessarily equate to high-quality code, lack of coverage does indicate low-quality code.
  3. Tests that don't actually test anything (better known as smoke tests). Tests should not just make sure that something runs without throwing an exception, they should also validate the state of things after the method completes. This means they should be testing the return type (and its properties!) and verifying any other state changes the method made.
  4. Poorly designed code. This is somewhat subjective, but there are a few easy things to watch for, such as duplicated code (copy-and-paste is BAD), things that are difficult to test, and things that are coupled to a large number of other classes.
  5. Obvious inefficiencies. Are they using a List where a Dictionary or HashSet would be better?

Aside from that, suggest changes anywhere that you see improvements. Doing so will improve the code base (making life for everyone easier) and will help the original author to become a better developer.

Once you are finished with the review, go ahead and commit the changes to source control. The author can then review your comments by searching for "PEER:" in their code. As each issue is addressed, the author should remove the tag from the code. If there is a question about a comment, the author should ask the reviewer for clarification. If there is a disagreement (which will happen from time to time), a senior developer should be consulted.

If applied correctly, Peer Reviews can help you become a better developer, help save your company money, and make life easier for everyone in the future.  Don't forget, if you're not doing peer reviews, let me know why not in the comments.

Tags:

blog comments powered by Disqus