An ever-present and often under appreciated part of software, code reviews are opportunities to
discuss changes with fellow members of your team before the changes are rebased onto the
branch for eventual deployment to a remote server. Whether you’re pushing up changes as the
author of your own work for others to review or you are the reviewer of changes pushed up by a
colleague, the goal is to solicit feedback and approval in order to create code of the best possible
quality. The end result is code that enhances the experience of paying customers and makes your code
easier to work with for your present team and into the future.
Even better, code reviews don’t have to be arduous or time consuming to be valuable (and profitable) if done well. Reviewing code also sharpens your skills, allows you to learn from others, satisfying those who are eternally curious, and levels you and your colleagues up in the process. You may find that systematizing and prioritizing your code reviews increases your creative problem solving. Remember, what we do as software engineers is as much a creative/artistic endeavor as it is a science. To quote from the earlier Farnam Street article:
Creativity rises from tranquility, not from disarray. Creativity is supported by a safe environment, one in which you can share and question openly and be heard with compassion and a desire to understand.
With the above in mind, let’s dive deeper into how to conduct meaningful code reviews.
Shown at the top of this article, Maslow’s Pyramid of Code Quality is an idea I stumbled upon originally authored by Charles-Axel Dein. I’ve made slight changes to the concept to match my experience, and think it provides a great visual to what is achievable if you grow and jell your team following its principles, including that software engineers should be:
When you are altruistic, you leave the code base better than what it was before, provide inspiration for fellow engineers to not only mimic but aspire to in their own work, and you become a shining example of what is achievable. Engineers that exhibit this level of care tend to be architects but can be seniors too.
You leverage good patterns and practices. Achieves functionality without sacrificing simplicity and conciseness. Demonstrates good practices for others to follow. Is a joy to work with, reference, and maintain. Senior engineers are the minimal level that tend to exhibit this level of the craft.
You demonstrate proper handling of the OWASP Top Ten security risks and much more. Mid to senior engineers tend to be the minimal level in which to be aware of these concerns.
Your code is written to be read and understood by a fellow human, including the associated Git commit explaining the reasoning behind the what, why, and how of the implementation. Modules, classes, methods, and variables are self-explanatory. Coding conventions are adhered to. Tests are brief, capture edge cases, and document the overall capabilities of the implementation. Mid engineers tend to be the minimal level where this skill is usually on display, but every now and then, younger engineers will break through.
Provides an implementation that does what it’s required to do, handles edge cases, and is decently tested. Unfortunately, this kind of code is usually hard to read and/or understand. You’ll generally discover such unreadable to code be written by a young engineer or someone who doesn’t grasp the language that well. All engineers start out at this level before progressing upwards. Sadly, a lot of code tends to not move beyond this point.
What Maslow’s Pyramid provides, in this context, is an illustration of the various gradations in code quality that can be achieved if you make these goals a team effort. The pyramid not only gives you a way in which to grade yourself and others but also to hold everyone accountable. Having this North Star is a big part of a healthy team and good engineering culture in general.
The following code review guidelines are broken down by section for improved readability but also link-ability should you need to point specific sections to others.
At a high level, the main objectives each code review are:
Joy - The experience of working with you, receiving feedback, giving feedback, and demonstrating a level of care influences others and encourages a collaborative environment that is fun to work in. Use the time spent code reviewing to build and reinforce trust amongst your fellow colleagues in order to achieve a jelled team.
Quality - Code reviews should ensure changes are of highest quality and adhere to team standards while enhancing the customer experience. By building code quality into your team’s culture, you’ll allow the team to grow and evolve while also creating an environment that attracts new talent. Not only does this practice save recruiting dollars but also avoids the brain drain of not retaining top talent.
Education - The conversation around reviews provides a chance for everyone on the team to learn more about the architecture, the product/service, and how each member of the team implements solutions. Code reviews are your chance to ask questions and learn how to be a better engineer, so take advantage! No one is too young or too old to learn something new, and you’ll discover constructive feedback is far more valuable than it is frightening.
Legacy - Your work, once rebased onto the
mainbranch after code review, is a legacy not only to yourself but to those who come after you. Your team’s Git repository is a permanent record and treasure trove of knowledge on how you evolved to where you are today. At any point in time, you can search through this historical knowledge base to remember how the code was made or learn from a fellow colleague’s work in order to add a new feature or fix and existing bug — all without having to contact that individual directly, which is very important if they’ve moved on. The history you preserve in your code reviews is your chance to leave behind a legacy which inspires others, the latter being is a key aspect of the Engineer’s Oath.
In addition to the objectives above, the following basic rules are worth following/enforcing when reviewing code:
Keep code reviews short and easy to review in order to reduce conflict resolution with upstream changes from the team:
Review your own code first before submitting to others. By ensuring you are confident in the overall architecture, commit messages, and implementation details, you’ll encourage higher quality feedback and show appreciation for the reviewer’s time.
Provide a high level overview that answers why the code review is necessary.
Provide a link to the issue that prompted the code review (if any) and any additional details worth highlighting to guide the reviewer through the process.
Provide screenshots/screencasts if possible.
Provide any ancillary notes or points of interest worth highlighting for the reviewer.
Ensure commits within the code review are related to the purpose of the code review and avoid mixing in changes that are ancillary to the primary objective of the code view. If you do have changes outside of the scope of the current code review, open those changes up as a separate code review instead.
Prefer code reviews with ~300 lines or less in order to keep the quality of the code review and defect detection high. This brevity also shows you value the reviewers time and attention away from their own work.
Avoid working on a large issues without first getting feedback in order to not overwhelm/surprise the maintainers. More discussion up front will prepare the maintainers and help ensure your work has a faster chance of acceptance.
Use all feedback as a chance to learn, teach, strengthen the bond of trust between you and your fellow colleague as well as avoid being cut by Hanlon’s Razor.
Respond to all feedback, even if minor (reactions or simple reaction statements are OK to skip, though). These conversations are a powerful opportunity to build trust and ensure your team members are heard.
Use face-to-face communication if a code review’s written discussion gets lengthy/noisy.
Create follow-up actions if additional features are discovered during a code review to avoid delaying code review acceptance. Return to the code review once the new actions have been logged.
The author, not the reviewer, should rebase the feature branch onto
mainupon approval. Otherwise, this robs the author of the satisfaction of completing their work. Additionally, the author might want to perform follow-up work, once rebased, that the reviewer might not be aware of.
Allow or even encourage your team to continue the code review discussion after the code has been rebased onto the
mainbranch — or closed — so the team can keep learning without holding up progress of work.
Instead of providing code samples and/or alternative solutions, consider approving the work in order to not hold up the author while you submit follow up code for review after the original work has been rebased onto
Avoid opening multiple code reviews where each child depends upon a parent. Each child causes unnecessary notifications for code that can’t be reviewed until the parent is reviewed. This is also taxing in terms of mental overhead and prevents reaching Inbox Zero quickly.
Avoid reviewing your own code before rebasing onto
main. Have another pair of eyes review your code first. In rare situations, you might have to rush getting code deployed to production. Ensure you leave a comment explaining why you needed to do this and the reasons for moving forward so quickly. This kind of behavior is generally discouraged but is sometimes necessary.
Avoid commenting on your own code review because you want all discussion (if any) around your code to come from the reviewer and not the author. Use your Git commits for capturing this information instead.
Avoid rejecting a code review outright. Rejection is too heavy handed especially when using emojis (detailed below). By not rejecting a code review but leaving a comment instead, you help build stronger bonds of trust by avoiding making someone feel like they are not trusted in addition to giving feedback that needs additional work.
Avoid setting the minimal number of required reviewers higher than one so as not to tie up the code review in endless debate.
Ensure the following criteria is met when rebasing your feature branch to
squash!commits are interactively rebased. Avoid rebasing these onto the
mainbranch! Git Lint can take care of this for you.
Ensure your feature branch is rebased upon
Ensure all tests and code quality checks are passing.
Ensure the feature branch is deleted after being successfully rebased.
Templates are great for providing a basic structure people can follow when submitting code for review or reporting an issue. These templates should clearly define what is required and what is optional when reviewing code. Otherwise, anyone can provide any structure they like or nothing at all which is a detriment to consistency. For example, consider the following templates which are provided for you via the Rubysmith or Gemsmith gems when building new Ruby projects:
Since the above templates are written in
Markdown, the comments (i.e.
never render, but they are none the less shown to instruct someone new to the team what is required
and what is optional. As a side note, these are quite valuable to test one’s attention to detail
when interviewing, as well.
Emojis are a powerful way to convey mood and/or liven up the conversation, especially when you are communicating asynchronously. Since you can’t see one another, it’s important to convey understanding with emojis — and preferably a select few. You can use more, of course, but here’s what I’ve evolved over the years:
Example: ⭐️ Looking forward to getting this released.
Emojis are only necessary when initiating new feedback but sometimes they can be useful when joining an existing conversation. A delicate balance is required in order to not overwhelm the author but when used judiciously, emojis are a great way to improve discourse between the author and you, the reviewer. The greatest benefit from this practice is that it:
Enables the author to immediately address blocking feedback while following up on non-blocking feedback afterwards.
Prevents the reviewer from having to constantly indicate feedback is either blocking or non-blocking due to the implicit nature of using the emoji. Now the reviewer can get right to the point and type less to boot!
For more details, I’ve spoken specifically about emojis and have an associated talk which might be of interest.
Here’s a breakdown of each emoji and how to use them effectively:
:information_source:) - Signifies informational feedback that is non-blocking. Can also be used to let one know you are done reviewing but haven’t approved yet (due to feedback that needs addressing), waiting for a green build, checking for further status updates, etc.
:thought_balloon:) - Signifies inquisitive probing that is non-blocking. Useful when asking questions and/or probing deeper into implementation details to learn more. Since this type of discussion is non-blocking, the conversation can continue even after the code review has completed.
:bulb:) - Indicates a helpful tip or trick for improving the code. This can be blocking or non-blocking feedback and is left up to the author to decide. Generally, it is a good idea to address and resolve the feedback.
:art:) - Signifies an issue with code style and/or code quality. This can be blocking or non-blocking feedback. It is up to the discretion of the author on how to address the feedback but encouraged that the feedback is incorporated or at least discussed. Generally, these situations are automatically detected via code linters but there are ambiguous situations which linters can’t catch.
:warning:) - Signifies an alert to the author. This can be blocking or non-blocking depending on the context and situation. It is encouraged, if the feedback can be addressed, to do the work. Otherwise, it can be a way to notify the author of upcoming changes or issues to watch out for.
:name_badge:) - Signifies detection of a misspelling with suggested correction. This is blocking feedback that is easy to correct.
:axe:) - Signifies text, code, commit, etc. that can be removed entirely since it not providing any value. This is meant to be blocking feedback that should be relatively easy to correct.
:stop_sign:) - Signifies an issue with the implementation’s architecture which might relate to performance concerns, hard to maintain code, security issues, etc. This is blocking and requires immediate correction. The reviewer should provide a suggested solution and/or links to patterns, articles, etc. that might help the author fix the implementation. Pairing is encouraged if feedback is vast and/or complex.
:mushroom:) - Signifies bonus points or extra credit for additional work that could be completed. Kind of like applying a power up in a game. This feedback is meant to be non-blocking but definitely encouraged if the author has time.
:sparkle:) - Signifies code that is liked, favorited, remarkable, etc. This feedback is non-blocking and always meant to be positive/uplifting.
:bow:) - Indicates thankfulness of the feedback received. This is non-blocking and always meant as a positive response to helpful feedback.
It’s important to a team’s cadence, workflow, general camaraderie, and respect to review feedback early and often. At a bare minimum, ensure you check in, at least three times a day on what your team is doing. As an example:
🌅 Dawn - Start your day by helping unblock your team by reviewing new work or moving existing conversations along.
☀️ Noon - After being heads down with you own work during the morning, make sure review code/feedback before or even after lunch.
🌃 Dusk - Before ending your work day, once again, make sure to address any/all outstanding code and/or feedback.
While the above is the minimum, realistically, as you pop up for air, you’ll most likely be curious about feedback on your own work and want to help other’s move their work along as well and check on feedback more often. Depending on the size and/or velocity of the team, checking too often can be distracting, so make sure to time box yourself and provide quality feedback without holding others up for too long.
Lastly, avoid letting code reviews linger more than a day. Otherwise, you risk hampering moral and diminishing the productivity of the team.
Use automation to ensure code reviews are consistent, of high quality, and swift to resolve. Much of the basics can be linted via Git Lint as code quality check on the anatomy of commit messages.
Each code review is also an opportunity to identify how to improve upon the process further. Tools like Git Lint are great for this and works with multiple programming languages.
While the following are more Ruby specific, these options might prove insightful on how to improve the tooling around your own programming language of choice:
Diffend - Ensures your gem dependencies don’t have known security flaws along with a bevy of other checks worth staying on top of.
Git Lint - Ensures Git commits are we structured and consistent which works great when building release notes via Milestoner.
Reek - Ensures your code doesn’t have any major code smells.
Caliber - Ensure your code doesn’t have style, layout, lint, metric, performance, and other issues. It’s a lot like Reek but more expansive and is built on top of RuboCop.
SimpleCov - Generates low level code coverage for which many tools can tap into but also provides useful statically generated reports.
If you’re stuck using GitHub for code reviews, you’ll want to apply the following changes to all of your projects:
Enforce a rebase workflow for all of your GitHub projects. You can do this via your project options
https://github.com/<owner/<repository>/settings) and editing your merge options for code
reviews as follows:
You’ll want to add branch protection rules for your
main branch. To do this, follow these steps:
Visit your branch settings. Example:
Click the Add rule button.
For branch name pattern, enter:
Check Require status checks to pass before merging.
Check Require signed commits.
Check Require linear history (pairs well with the merge options mentioned above).
With the above applied, you should have the following result:
Applying the above changes will help maintain a clean Git history.
Viligant Mode provides an additional layer of security and verification for all Git commits and tags. You can enable this setting via the following steps:
Go to the profile settings of your account.
Click on SSH and GPG keys.
Check Flag unsigned commits as unverified.
Upon successful completion, you should end up with the resulting configuration:
Should you want to dive deeper into the subject of code reviews, the following might be interest:
Don’t Let Code Reviews Destroy Your Team Morale by Tammy Xu.
Disadvantages of Pull Requests by Tomasz Wróbel.