Code Review Process

Owned by Medha Parlikar (Unlicensed)
Last updated: Jul 06, 2018 by Kelly FosterVersion comment

Read this page once when you first join the project, read it again when you submit your first PR, and read it again when you receive your first request to review code.

Why Code Reviews are Important:

When performed with diligence and effort, code reviews have been proven to be the single best way to eliminate defects in production code.

Guidance

Code reviews are not an approval process. They are a way of having a conversation about the work and to use the collective skill of the team to improve code readability, quality, and finding bugs. Do not let egos get involved. Code reviews are not about criticism.

When submitting code for review:

  • Assure PRs are sized for manageable and successful review. Limit your pull requests to 200 lines of code, excluding test code. 
    • Once you have an idea of how the code in a PR will look, you should immediately begin thinking about how to break it up for the reviewer.
      It's also a good time to reach out to a reviewer to let them know we'll be sending something their way and talk over how we're going to structure the set of PRs.
    • One way is to break it up by dependency: we issue a PR for new code with no dependencies, then a PR for code that depends on those, then a PR for stuff that depends on that, etc.  Once all the new code is in, then we make the switch from depending on old code to new.
    • While it feels like a hassle in the middle of coding to stop and do something else, it will actually take less time overall than to dump a large pull request on a reviewer.
  • When you create your PR, assign someone to review it.
    • Select a reviewer based on who you think will do the best job. 
    • Don't assign more than two people to your code review, because then we get the 'bystander effect' when there are lots of reviewers.
  • Sign your commit. See How to sign commits to rchain/rchain for instructions.
  • In general, try to get your code reviewed every 48 hours.  
  • It's okay to have several code reviews in a sprint.
  • It's okay to have several code reviews for a single ticket.
  • If you are writing a lot of boilerplate code, you could drop more code in a single PR for review if desired, because the reviewer could learn the syntactic pattern.
  • Don't hold on to your code until the issue is resolved, there is no need to do that.
  • You can use Pseudo code and include your design, the code doesn't have to work.  Comment it out if needed, because we don't want to break tests either.
  • It is harder to review code than it is to write it.  Please write more comments and description outlining your work.  Reviews take more effort - please write a paragraph about your code.


When reviewing code:

  • When you receive a code review request, show the courtesy of responding within a day on when you can complete the code review.
  • You need to review every line of code.  You should be able to understand the code.
  • Provide feedback!  Is the code expressive and easy to understand?   

Examples of some completed Code Reviews: