The Code Review Guide v1.0

Oscar Siniscalchi

Director of Engineering

While this is mainly for Ruby developers, we believe in a transparent process and since our processes are some of the best in the game, it’d be a shame not to share. Below you’ll find a checklist for how we do reviews for any PR at Neon Roots.

Keeping It Clean Through Code Review

Code review is a very important step in our company production workflow. It allows us be in touch with the current projects regardless of what we are working on, and it makes the code look better and uniform. The ultimate goal is to make the code look like it was written by one person so that any team member can join a project with minimal ramping up effort.

Also, it keeps the code clean and maintainable, and trust me, when you have to work on code you did four years ago and you start considering changing your career choice rather than having to fix a bug, it’s because you did not follow good practices.

Plus, on a more pragmatic level it also makes sure we spot any potential bugs before we even push the code to development. This checklist has the goal of making the review an automated and easy task that ensures the absolute best quality. So follow these steps and make sure ALL are covered before you give the thumbs up.

Build

  • Does the build succeed in the CircleCi?
  • Are there any exceptions added to the code analysis? Is it necessary?
  • Is the PR too big?
  • Does the PR include a link to the card on Trello and a description of what was addressed?
  • Does the PR address a single independent problem?

Tests

  • Does the new feature include functional tests?
  • If a service is created, does it include unity tests for all methods?
  • The test cases are readable (should read like a sentence when failing).
  • Are all the Acceptance Criterias covered in tests?

Resolution

  • Does the controller have too much Biz logic?
  • Is there a similar problem solved in a different way on the same project?
  • Are the names of the methods and classed added understandable?
  • Does this make the models or the controllers look fat?
  • Is it an over-engineered solution for a simple task?

Style

  • Is everything aligned correctly?
  • Are we using “instead of” unnecessarily?
  • Are we using old hashes?

Keep in mind that EVERY question is important. Cleaning up code is no time to cut corners, but this checklist is going to give you the squeaky clean code that you’ll never know passed through countless hands.