Three Suggestions for Successful Code Reviews

We’ve all been a part of bad code reviews. Both as the developer and as the reviewer. If you haven’t, you’re extremely lucky! Code approved with zero comments seconds after the pull request was submitted are just as frustrating as ones with an entire novel, possibly with several copies of the same chapter, written across dozens of comments with no context. We’ve written internal guidelines to remind us that reviews are an important part of the process to writing good code. Not only because we want the code we’re looking at now to be better, but because we want to help each other be better developers as well.

What to do long before reviewing any code

• Shake the boring parts out of the process!

  Automate everything you can! Linters, code analysis, style and spell checkers, unit tests and more can be added to your CI process. Let the computers do what they do best: do repetitive mindless tasks. Either add these over time, or find a CI system like EmbedOps that will give all of this automation to you out of the box.

• Create a common language for review comments

  Not every comment is equal. Some things MUST be fixed before the code can be merged, some things are merely nice to have. And some things are personal preference or contingent on a question being answered! All of this feedback is useful in its place, but without clarity on what kind of feedback is being provided, it is hard for the developer to prioritize fixes, and can lead to hurt feelings or unnecessary arguments or frustration.

  Come up with a team shorthand to make your feedback more clear. Hashtags, emoji, or acronyms all work just fine.

• Have templates to make the process easy to follow

  Templates for reviews will remind both the developer and the reviewer of the steps necessary for a complete review. Like automating, it gets the boring bits out of the way. Checklists are used in safety critical environments so no one forgets important details even when things get chaotic or stressful. They are just as important for developers, because who of us hasn’t skipped a step on accident or been tempted to make exceptions to meet a deadline?

• Make sure junior developers are also reviewers

  In a previous post, we talked about how reading code is a separate skill from writing code. Don’t shortchange the team’s younger developers by not allowing them to practice their code reading skills! Getting code reviewed by someone junior to you also gives you a chance to get a fresh perspective on your code and reflect on the habits you may have picked up over time, as well!

What to do in a code review

The following are items in our review templates! It seems like a lot, but since there is a clear checklist on what makes a good review, they are quicker than you’d think while still being thorough.

• Look at individual commits

  • This is especially helpful if there are many changes in the overall diff. They happen sometimes. Hopefully not often, but every now and again it’s necessary to touch more than you’d like.
  • The commit messages should give a story of how the code was written. Consistently reviewing these can help the team be better developers as well.

• Look for differences that aren’t whitespace or formatting

  • Set the option to ignore whitespace if available. GitHub and GitLab have this option.
  • If reformatting changes are needed, get in a team habit of putting them on a separate commit to isolate them.

• Where are the comments?

  • In code, of course, especially if documentation is required on your project, but also on commit messages, in the PR, and when necessary, in the README.

• Don’t add/modify the code

  • you’re wearing your reviewer hat now. Any changes to the source should be made by the developer. You can, however, make code suggestions in the comments if that makes your feedback more concise or easier to read.GitLab has a feature to make code suggestions in reviews. They allow the developer to easily insert these directly into the source code if they agree with the suggestion.

• Ensure the requirements are met by this code

  • We often use the requirement number or the issue ID as part of the branch name to tie these together. Putting an item for the developer to fill out in your template would also be acceptable.
  • On the other hand, is the code only fulfilling this requirement, or are other changes included as well? It’s okay to fix bugs, especially when very relevant to the requirement being satisfied. But putting those fixes on their own branch can make the code history easier to track, easier to find and fix other bugs in the future, and keep the code review short and easy.

• Check for best practices

  • This is the step where you look for anti-patterns, spaghetti code, re-inventing wheels, common mis-uses of memory or other resources, and other things that your automation can’t catch.
  • If you’re finding the same kind of mistake more than a couple of times, can it be automated? Make a note to follow up on it at your next team meeting.

• Has the code been tested?

  • Are there new unit tests written for this code, especially if it’s a bug fix? Do the tests cover the boundary conditions in the requirements?
  • Have functional tests been written and executed?
  • Has this code and the functional tests been run on the target system? What were the results?
  • Do you get the same results when you run the test? Are any steps being assumed in the writeup? Is everything being tested that needs to be?

How to give kind and helpful feedback

You might expect to see a “what not to do” section here, but there are plenty of those that do the job well. We find it more helpful to outline what kind and useful feedback looks like.

• Communicate humbly

  • You often won’t have as in-depth knowledge of code as developer, especially if you are new to the codebase or have been working on unrelated tasks. Remember that your suggested fix may introduce other issues! At DojoFive, we provide consulting services, and our assumption is that the client knows more about their codebase than we do. Something that may not seem right to our team may make perfect sense for theirs. As the reviewer, it helps to communicate your suggestions with humility and from a place of mutual learning.
  • Don’t blame anyone. The developer you’re reviewing, the person who introduced a bug, the requirements writer, the client, or yourself! We’re all human. Find the solution, celebrate when the team learns and grows, and move on.

• Keep it concise

  • If your comment gets over a couple sentences, maybe a code suggestion would say more with less, or maybe it’s time to prioritize what to discuss.

• Sometimes it’s best to talk it out.

  • Are there an unusually large number of changes?
  • Do you have a question that is similar to “Can you tell me more about your solution to ____ ?”
  • Does the review look like it is going to take you, or has taken you, hours to complete?
  • Has the review already bounced back and forth a couple of times?
  • If you answered yes to any of these questions, it’s time to get on a call.

• Give feedback in order of importance

  • If you’re writing a report, make sure the most important items to address is at the top, least important at the bottom. If it’s a large report, which often happens when you’re auditing an entire codebase rather than PRs, split feedback into priority levels and give a summary of why you did so as an introduction. Things that are likely to break are more important than things that are merely non-optimal.
  • This is where that common language above comes into play! It’s hard or impossible to reorganize comments on most git interfaces, so give the developer a way to prioritize what to focus on.

• Keep your own notes of things you’ve seen that are out of scope of the review

  • You may make a connection later in the review, or it may come up later in the project.
  • But to keep things concise, remember that these are YOUR notes, not review comments. If you see something out of scope that needs communicating, consider if a new issue, or adding to an existing issue is a better place for that feedback.

We’re always improving our processes here at Dojo Five. If you have suggestions for us to improve our code reviews, we’re happy to talk about them! Don’t hesitate to reach out and chat with us on LinkedIn or through email!