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?"
Certainly there are derivative reasons, but I find myself coming back to these two:
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:
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.
Once we know what it should do, we can investigate how the author verified it.
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?
The tests should be reviewed like any other code!
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:
Complexity happens naturally, simplicity takes work and creativity. A second opinion can easily identify simpler alternatives.
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!
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:
Using code reviews to catch repetitive issues is going to be a problem when the code review misses it or fails to happen.
One overlooked area of feedback is the commit log. Look for problems like
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.
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.
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.
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.
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 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.
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.
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.
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.
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.
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.