API Documentation

Code Reviews

Code reviews are an integral part of the life cycle of our code. They serve multiple purposes and every developer, regardless of level is expected to take part in the code review process.

Code review purposes

  • Help identify and prevent bugs
    Bugs in software is something that will always exist. There's no way to prevent it, only reduce the chance of it occurring. One of the primary concerns of a code review is to help identify bugs, or other issues that the author might have missed.
  • Helps prevent security issues
    Similar to bugs, but maybe more important, security issues pose a serious threat to all code bases. The more eyes on a change the more chances of spotting any issues which may break any security we have in place.
  • Preserves the common code style
    By keeping a consistent code style across all projects and developers, a code review is made easier because we don't need to get distracted by minor stylistic issues and can focus on the bigger picture.
  • Helps share ideas about how to solve issues
    Programming is a never ending story of continual self improvement. Whether you've been programming for 1 month or 20 years, code reviews allow you to learn easily from other developers directly through the code. You can see how patterns are implemented, how refactors are performed, how bugs are resolved and so on.
  • Helps to share understanding of a particular feature
    Our industry is complicated. Code reviews allow you to figure out how a particular part of the business works by seeing it in a way we all know - code, rather than business speak.
  • Promotes communication within a remote team
    As a remote team, communication is key to making sure that we're programming features correctly, making sure we all feel like part of an actual team and help share ideas. Code reviews help you to develop communication within the team. You are forced to explain your ideas in a way that is easily understandable.

There are many more reasons for performing code reviews but these are some of the reasons we focus on.

Additionally, even if a PR has already had a review, don't let that stop you giving it an additional look over. There is no upper limit on the number of reviews a change can receive.

Before sharing the Pull Request

Before sharing a branch for code review, try think how to reduce the amount of time needed on the review.

  • Perform a code review yourself, as if you were reviewing another developers work. Leave comments, suggestions etc.
  • Make sure tests are passing before requesting a review
  • Put a link to the pull request in the ticket

Submitting a Pull Request

When submitting a code review, you should try to keep in mind the target audience of your review. They will generally be other developers and QA.

The pull request description should be only as long as needed, including key information that will be useful to reviewers or testers. This may include:

  • Key areas of the PR that may need special attention. This might be because it might be more likely to contain a bug, contains changes that other developers may need to consider in future PRs and so on.
  • Any changes that need to be made to the deployment process, or production / UAT environments before being able to deploy
  • Links to any other branches that need to be merged in addition to the branch being reviewed - IE an API update that also updates the SDK.
  • A link to the ticket
  • Any instructions, hints or tips if a manual test is required
  • Explanations as to why something was done a certain way, so reviewers take that into account during the review.

Helping reviewers

As important as any code changes are, its also important to be able to get code out safely and in time. To do this, you have to make your changes easy to review.

  • Avoid large PRs
    Try to keep changes as small as possible. The less amount of changes, the easier it is to review. A 10 line PR is going to get more reviews and faster, than a 10,000 line PR. Large PRs are also hugely more likely to contain bugs, making them riskier to deploy, and more stressful to review. If you need to, split a ticket into smaller tickets, which result in smaller changes.
  • Include Tests
    To reduce the feeling of "responsibility" of being the reviewer of a change that breaks, include automated checks that test your change.
  • Include a link to the ticket
    Reviewers may find it useful to understand the context of a change. The reason for the change will always be found in the ticket, so including a link to the ticket helps the reviewer understand the problem we're trying to solve, and may also identify others in the business who requested the change who may be able to answer any questions.
  • Be Verbose
    When making changes, be verbose in communicating the changes you're making in the code. Try to avoid complicated code, refactor big chunks of code into smaller functions with understandable names.
  • Avoid out of scope changes Avoid making changes unrelated to the ticket such as refactors. Make the change you need to, create a ticket for any refactors you think are needed along the way, and address them separately.

Performing Reviews

A review can be performed by any member of the team, regardless of experience. PRs are never merged until there is a high level of confidence that the change is safe. So whether you have no experience within a specific project, or you wrote the entire code base, you should be performing reviews where possible.

When performing code reviews, you should be keeping an eye out for:

  • Bugs - helping to identify any bugs which the author may have missed.
  • Identifying complicated areas of code. If you cant suggest what to do instead, highlight that area as being complicated. Another reviewer may have ideas how to simplify it.
  • Code smells - watch out for smells in the code. If you know the name of them, great. If not, a simple "this feels a bit weird" is fine.
  • Improvements - If you think code could be improved, speak up. You may have a better way of doing something the author didn't think of.
  • Try to be verbose in your comments. If something doesn't make sense, explain why, it may make perfect sense to a developer who's been working on it for hours, but in 4 months when we revisit, we'll also need to understand it then.
  • Understand that not every suggestion will be implemented.
  • Making sure the branch addresses the issue within the ticket.

You don't need to catch every single bug, issue or smell. To reiterate, reviews are a way to increase safety, not eliminate all issues, that's just an added bonus if it happens.

In addition, code reviews don't always need to feel negative. If you like a particular addition or change, mention it in a comment.

Review Etiquette

We are a team of brilliant developers, testers and support users. No one here would intentionally set out to start arguments or belittle anyone else. Code reviews should be seen as a positive process, not a negative one.

  • Be polite
    When writing comments, don't be rude or abrasive. Treat others how you'd want to be treated.
  • Praise
    Where appropriate, provide praise where somethings been done well.
  • Don't take comments personally
    A code review is about the code only. No one will be judging you as the author. Comments are there to help you, and the business.
  • Consider pressures
    When performing code reviews, be aware of the pressure the author may have been under to fix the issue, or work in an unfamiliar area of code.
  • Reply to comments When and where appropriate, take the time as the author of a change to reply to comments. If you're going to decline a suggestion, communicate why.
  • Thank reviewers Thank reviewers. Kindness costs nothing.