Code Review Guidelines

This guide contains advice and best practices for performing code review, and having your code reviewed. Some benefits we can find when working on a Merge Request based workflow are:

  • Check if the code is effective, understandable, maintainable, and secure. This concepts aren't usually covered by automated testing.
  • Learn from others code. While reviewing code we can learn new things and improve our coding skills.
  • Code review can have an important function of teaching developers something new about a language, a framework, or general software design principles. It’s always fine to leave comments that help a developer learn something new. Sharing knowledge is part of improving the code health of a system over time.

Code Reviews workflow

When working on a new feature or hotfix we cannot directly merge into the main branch. See our git workflow explainer for further details. The review workflow is:

  1. The team member works on a new feature and creates a PR.
  2. The PR must comply with the proper description. Checkout our standard gitlab templates.
  3. The CI must run successfully on the PR branch (linting, tests, etc).
  4. Another team member must review the PR following the review checklist.
  5. Once approved the PR is ready to be merged:
    • keep your commit history tidy and linear.
    • do not use git fast forward.
    • if you want to, you can squash your commits.
    • delete the source branch to avoid polluting the repo.

Credits and Bibliography

The following guide is based on two awesome resources:

In the following sections some highlights will be shared although you are encouraged to read both guides thoroughly in order to understand the concepts.

As an author

Getting your merge request reviewed, approved, and merged

You are strongly encouraged to get your code reviewed by a reviewer as soon as there is any code to review, to get a second opinion on the chosen solution and implementation, and an extra pair of eyes looking for bugs, logic problems, or uncovered edge cases. The reviewer can be from a different team, but it is recommended to pick someone who knows the domain well.

For approvals, we use the approval functionality found in the merge request widget. Reviewers can add their approval by approving additionally.

Getting your merge request merged might require more than one approval.

Best practices

Please keep in mind that code review is a process that can take multiple iterations, and reviewers may spot things later that they may not have seen the first time.

  • The first reviewer of your code is you. Before you perform that first push of your shiny new branch, read through the entire diff. Does it make sense? Did you include something unrelated to the overall purpose of the changes? Did you forget to remove any debugging code?
  • Be grateful for the reviewer’s suggestions. (“Good call. I’ll make that change.”)
  • Don’t take it personally. The review is of the code, not of you.
  • Explain why the code exists. (“It’s like that because of these reasons. Would it be more clear if I rename this class/file/method/variable?”)
  • A MR description is a public record of what change is being made and why it was made. It will become a permanent part of our version control history, and will possibly be read by hundreds of people other than your reviewers over the years.
  • Related to the previous item, you must use the Merge Requests template available in Atix Docs Templates.
  • Create small Merge Requests, i.e. one self-contained change.
  • Extract unrelated changes and refactorings into future merge requests/issues.
  • Seek to understand the reviewer’s perspective.
  • Try to respond to every comment.
  • The merge request author resolves only the discussions they have fully addressed. If there’s an open reply, an open discussion, a suggestion, a question, or anything else, the discussion should be left to be resolved by the reviewer.
  • Push commits based on earlier rounds of feedback as isolated commits to the branch. Do not squash until the branch is ready to merge. Reviewers should be able to read individual updates based on their earlier feedback.

As a Reviewer

Reviewing code

Understand why the change is necessary (fixes a bug, improves the user experience, refactors the existing code). Then:

  • Try to be thorough in your reviews to reduce the number of iterations.
  • Communicate which ideas you feel strongly about and those you don’t.
  • Identify ways to simplify the code while still solving the problem.
  • Offer alternative implementations, but assume the author already considered them. (“What do you think about using a custom validator here?”)
  • Seek to understand the author’s perspective.
  • If you don’t understand a piece of code, say so. There’s a good chance someone else would be confused by it as well.
  • After a round of line notes, it can be helpful to post a summary note such as “LGTM 👍” (looks good to me), or “Just a couple things to address.”
  • Assign the merge request to the author if changes are required following your review.
  • Set the milestone before merging a merge request.
  • Avoid accepting a merge request before the job succeeds. Of course, “Merge When Pipeline Succeeds” (MWPS) is fine.
  • If you set the MR to “Merge When Pipeline Succeeds”, you should take over subsequent revisions for anything that would be spotted after that.
  • Consider using the Squash and merge feature when the merge request has a lot of commits. When merging code, a reviewer should only use the squash feature if the author has already set this option or if the merge request clearly contains a messy commit history that is intended to be squashed. If you are about to use the squash option make sure that no one else in the team created a new branch from the one to be squashed or is intending to use the same branch to add more commits. If you use squash and the branch is still being used, merge conflicts are guaranteed to happen even if that branch is not used to branch out from it, for example if a reviewer has checked it out to review it.

The right balance

One of the most difficult things during code review is finding the right balance in how deep the reviewer can interfere with the code created by a reviewee.

  • Learning how to find the right balance takes time.
  • Finding bugs and improving code style is important, but thinking about good design is important as well. Building abstractions and good design is what makes it possible to hide complexity and makes future changes easier.
  • Asking the reviewee to change the design sometimes means the complete rewrite of the contributed code. It’s usually a good idea to ask another reviewer before doing it, but have the courage to do it when you believe it is important.
  • There is a difference in doing things right and doing things right now. Ideally, we should do the former, but in the real world we need the latter as well. A good example is a security fix which should be released as soon as possible. Asking the reviewee to do the major refactoring in the merge request that is an urgent fix should be avoided.
  • Doing things well today is usually better than doing something perfectly tomorrow. Shipping a kludge today is usually worse than doing something well tomorrow. When you are not able to find the right balance, ask other people about their opinion.

Speed of Code Reviews

Why Should Code Reviews Be Fast?

When code reviews are slow, several things happen:

  • The velocity of the team as a whole is decreased. Yes, the individual, who doesn’t respond quickly to the review, gets other work done. However, new features and bug fixes for the rest of the team are delayed by days, weeks, or months as each CL waits for review and re-review.
  • Developers start to protest the code review process. If a reviewer only responds every few days, but requests major changes to the CL each time, that can be frustrating and difficult for developers. Often, this is expressed as complaints about how “strict” the reviewer is being. If the reviewer requests the same substantial changes (changes which really do improve code health) but responds quickly every time the developer makes an update, the complaints tend to disappear. Most complaints about the code review process are actually resolved by making the process faster.
  • Code health can be impacted. When reviews are slow, there is increased pressure to allow developers to submit CLs that are not as good as they could be. Slow reviews also discourage code cleanups, refactorings, and further improvements to existing CLs.

Since unblocking others is always a top priority, reviewers are expected to review assigned merge requests in a timely manner, even when this may negatively impact their other tasks and priorities.

How Fast Should Code Reviews Be?

If you are not in the middle of a focused task, you should do a code review shortly after it comes in.

One business day is the maximum time it should take to respond to a code review request (i.e. first thing the next morning).

Even if it sometimes takes a long time to get through the entire review process, having quick responses from the reviewer throughout the process significantly eases the frustration developers can feel with “slow” code reviews.

Speed vs. Interruption

There is one time where the consideration of personal velocity trumps team velocity. If you are in the middle of a focused task, such as writing code, don’t interrupt yourself to do a code review. Research has shown that it can take a long time for a developer to get back into a smooth flow of development after being interrupted. So interrupting yourself while coding is actually more expensive to the team than making another developer wait a bit for a code review.

Instead, wait for a break point in your work before you respond to a request for review. This could be when your current coding task is completed, after lunch, returning from a meeting, coming back from the kitchen, etc.

Checklist

  • [ ] Format and Info

    • [ ] Is the merge request small enough? Can it be split in different MRs?
    • [ ] Am I trying to merge to the proper branch? is the source branch ok?
    • [ ] Have I updated the source branch with the latest destination branch changes (git pull --rebase remote dest)? Do I have conflicts?
    • [ ] Is the MR title and description ok? Is the context well explained?
    • [ ] Did I include links to documentation (UATs, Test Cases, etc)?
    • [ ] Are the commits following conventional commits standard?

  • [ ] Correct behavior:

    • [ ] Does the functionality meet the requirements?
    • [ ] Are the provided tests ok? Do they cover corner cases?
    • [ ] Can I execute the provided code and run some manual tests to make sure the changes were properly integrated?
  • [ ] Code checks:
    • [ ] Syntax (variable naming, file names, code clarity, etc).
    • [ ] Is there any alternative solution that might be easier to read or more performing?
    • [ ] Are the new modules or pieces of code well located?
    • [ ] Have any architectural changes been introduced? Do they make sense?
    • [ ] Does the functionality include proper logs?
    • [ ] Is the code well documented?
  • [ ] External dependencies:
    • [ ] Check the introduced dependencies health (repository stars, open issues, tests, last activity).
    • [ ] Version should be stable and fixed.
    • [ ] Check the library is continuously updated and not abandoned. More in Picking_a_library
  • [ ] Non functional requirements:
    • [ ] Performance restrictions
    • [ ] Scalability
    • [ ] Concurrent issues are not present (deadlocks, race conditions, etc).
  • [ ] Deployment:
    • [ ] Are the changes retro-compatibles?
    • [ ] Does it need data migration scripts? Are those provided?
    • [ ] Does it affect the deployment workflow?
  • [ ] Security:
    • [ ] Does it include sensitive data that should not be committed and pushed to the repository? (keys, api tokens, secret configs, etc)?
    • [ ] Is authentication and authorization configured correctly?
    • [ ] Are security standards considered? (HTTPS, CORS, XSS, SQL injection, etc)
    • [ ] Is any information stored in plain text but should be protected? (Obfuscated or Encrypted)
    • [ ] Does the logs leak sensitive info?

General Advices

  • Accept that many programming decisions are opinions. Discuss tradeoffs, which you prefer, and reach a resolution quickly.
  • Ask questions; don’t make demands. (“What do you think about naming this :user_id?”)
  • Ask for clarification. (“I didn’t understand. Can you clarify?”)
  • Avoid selective ownership of code. (“mine”, “not mine”, “yours”)
  • Avoid using terms that could be seen as referring to personal traits. (“dumb”, “stupid”). Assume everyone is attractive, intelligent, and well-meaning.
  • Be explicit. Remember people don’t always understand your intentions online.
  • Be humble. (“I’m not sure - let’s look it up.”)
  • Don’t use hyperbole. (“always”, “never”, “endlessly”, “nothing”)
  • Be careful about the use of sarcasm. Everything we do is public; what seems like good-natured ribbing to you and a long-time colleague might come off as mean and unwelcoming to a person new to the project.
  • Consider one-on-one chats or video calls if there are too many “I didn’t understand” or “Alternative solution:” comments. Post a follow-up comment summarizing one-on-one discussion.
  • If you ask a question to a specific person, always start the comment by mentioning them; this will ensure they see it if their notification level is set to “mentioned” and other people will understand they don’t have to respond.