6.14.2013

The value of code reviews

Next week I'm going to pair with Sean Massa to talk about Code Reviews in Node. In lieu of this I thought maybe I should write down some of the things I look for in a code review. My perspective on this topic is definitely not unique, but after years of doing these I think I have some ideas about what works and especially what doesn't! My first inclination was to dive in and organize anything that comes to mind into tips or patterns. Before I do that though, first I wanted to ask myself: "Why do code reviews?"

Why do code reviews?

Certainly there are derivative reasons, but I find myself coming back to these two:

  • Improve quality
  • Shared learning

I'm sure there are other possible reasons, but this should be enough to get started. To avoid duplication, I found these fantastic posts that justify why these are important:

Improving Quality

Ask "What were the requirements?"

Misunderstandings and miscommunication are a leading cause of problems in software development. Especially with the prevalence of the mediating involvement anti-pattern. Writing code is a translation of the original idea and as with any translation, runs the risk of "lost in translation."

If you're a part of the team, revisit the topic to make sure you validate your assumptions. If you're not part of the team, invest some time in understanding the purpose before doing too much code review.

Once you know the requirements, watch for discrepancies.

Ask "How do we know this works?"

Once we know what it should do, we can investigate how the author verified it.

  • Manual Verification? - If the author used manual testing, it's possibly worth keeping track of that somewhere so the same manual tests can be repeated in the future. Study or inquire about these.
  • Automated Tests? - Review them to see if they match the requirements.
  • On faith? - Even if it's simple, it doesn't hurt to manually verify the happy paths. If it's complex, consider automation.

Ask "How can we improve testing?"

  • Are there missing scenarios?
  • Is there an elephant in the room after implementing a feature - things that haven't been discussed that could be a problem?
  • Is there a way easily automate testing?
  • What testing scenarios could be added?

Ask "Do the tests help us communicate that we are done?"

Does our testing give everyone on the team, non technical people included, confidence that we are done? If not, how could it? Where would this be most beneficial?

Ask "Are tests explicit?"

  • Can you easily identify the AAAs (Arrange, Act, Assert)?
  • Are the tests difficult to understand?
  • Are the tests concise?

The tests should be reviewed like any other code!

Ask "Does the code make sense? Is it easy to follow?"

If the code is difficult to understand now, imagine how difficult it will be in a few months when anyone, the author included, is re-visiting it.

Here are some simple refactorings that can be done:

  • Rename abbreviated terms
  • Reorder code
  • Replace comments with composition
  • Extract class from parameters

Ask "Is this code simple?"

Complexity happens naturally, simplicity takes work and creativity. A second opinion can easily identify simpler alternatives.

  • Prefer explicit over magic
  • Prefer less code
  • Prefer declarative code, separate what from how

Focus on what matters

There's only so much time in the day. What little time you have for code reviews, make sure it's well spent. This requires prioritizing what you review. As I've said before, ask "Does it really matter?". Focus on complexity, the business domain, challenging problems etc. Don't worry about administrative CRUD interfaces. Focus on areas you routinely have problems. Focus on what keeps you up at night!

Identify repeat issues and do a root cause analysis

Keep track of common problems, instead of relying on the review to fix them, focus instead on using reviews to identify them. Then, work as a team to address the problems. Some examples:

  • Code formatting, style, and layout inconsistencies - perhaps invest in a tool to do this for you
  • Missing error handling - perhaps identify patterns or languages to get around these issues
  • Unused code - perhaps invest in a static analysis tool

Using code reviews to catch repetitive issues is going to be a problem when the code review misses it or fails to happen.

Ask "Does the commit log help organize the changes?"

One overlooked area of feedback is the commit log. Look for problems like

  • Files committed separately
  • Infrequent commits
  • Commits based on components of technology and not business value. IE: a commit for the controller, a commit for the model, a commit for the database changes and a commit for the view. Instead it's often much more useful to have these organized into one commit per use case/story/feature, especially when doing code reviews!

Shared Learning

This normally happens as a consequence of doing code reviews. However, if we are cognizant of it we can maximize the benefits and actually avoid doing things that can inhibit learning.

Review together

Do the code review with the author involved, there's no faster way to learn than having both people sit down together. Obviously this can't always happen, but for significant functionality it's probably a wise idea.

Don't rely on reviews to catch your mistakes

There's a trade off between wasted time reviewing and not enough, there's no guarantee every piece of code will be reviewed. Instead of relying on a review to catch mistakes, try to minimize the mistakes that you think are likely so the review instead focuses on highly valuable topics like simplicity, readability, verification and other areas where you can learn from others. Do the very best work you are capable of so you can learn new ideas from the reviews.

Avoid doing the work for the other person

Code reviews should be a process of feedback and discussion. Obviously there are times when you're refactoring and you just have to do it yourself. If you are intentionally doing a code review, try to involve the author in the changes.

In my experience, when I make the changes myself and ask the person to review them, I see more repeat behavior of the same problems, not a reduction. As a reviewer, be very cognizant of this risk.

Review sooner rather than later

The longer you wait, the more likely things that are important to discuss will be forgotten and the more likely the code will be in production causing problems anyways. Quick feedback is always beneficial. The longer you wait, the less beneficial a code review will be.

Pair programming

Pair programming is like the best form of code review, it happens instantaneously. Of course, make sure you use this judiciously. For example, there's probably no reason to use it on an administrative CRUD interface.

Don't use reviews to fix problems from people who don't give a ...

When people repeatedly make negligent changes and just don't care about the quality of the code, the code review won't help. I've made the mistake of trying to cleanup after this behavior and it never helps.

There's no way code reviews will solve this problem, they can only identify when it may be happening. Don't try to be the hero that always reviews this person's code, you will be very unhappy. Perhaps do some pair programming with the person to see if it's negligent or accidental, and then deal with it outside the code review process.

Assume good intentions

Unless you are dealing with someone who doesn't give a ..., assume the person does until proven otherwise. This will lead to a much more productive learning experience.

Ask why if something doesn't make sense

Sometimes you see something you don't understand, instead of inferring it's not necessary or was a "bad decision." Ask why it's there.

Avoid taking offense and try not to be offensive

We all take pride in what we do, keep it constructive. Likewise, if someone is coming across offensively, point it out politely. It's tempting when you get repeat behavior or rudimentary problems to become frustrated, but remember these can be a sign of a problem in the process, it's often a sign that people are rushed. Sometimes code reviews cannot fix the underlying problem.

TLDR

At the end of the day, a code review is an opportunity for feedback. We do them to improve quality and to learn. The specific techniques will be subjective but keep the end goal in mind. Leverage techniques of testing, refactoring, automation, design and every other aspect of software development in the process.





comments powered by Disqus