Some guidelines for doing a code review

Code Review Guidelines.md

Here are some general guidelines to consider when reviewing a pull request:

• Is the code covered by tests? *
• Are there any tests cases that are missing? *
  • Unit tests - for any model changes *
  • Acceptance tests - ???infrastructure needed
  • Integration tests - for any front end behavior change *
  • Black box testing? infrastructure needed
  • Controller tests - for any controller changes *
• Rake / release tasks - manually run them? *
  • Make sure they’re extracted into a class and tested *
• Breaking the rules - you need to convince your pair and the code reviewers. *
• Did the CI build pass? *
• Does the code have a neutral or positive impact on code metrics? *
  • Or if the impact is negative is it justified?
• Do you have any concerns regarding the security implications of the PR? *
• Does it solve the right problem? *
  • If you're not sure, read the ticket. There is a ticket, right?
• Does the code conform to our style guide? *
• Are there any glaring errors or omissions? *
  • is there a missing path through the code
  • do we have an if with no else
  • Are we handling any nils that could come in
  • Are we throwing exceptions away
  • Are there any syntax errors
  • Is there a misspelled word
  • Did something get subtracted when it should have been added
  • Are we using a float when we should be using a int
  • Are there magic numbers in the code
  • Did protected keyword get use correctly
  • Are their private methods in the public interface
  • Is there a comment that should be code
• Is the code well factored? *
• Is it code you are going to want to maintain? *
• Does the code run locally? *
• Why are so many people willing to put a thumbs up, but not willing to merge?
  • Merges should be done by the pair that picks up the ticket.
• If you think it’ll take less than 10 minutes to make a needed change then do it yourself as part of code review
• If you think it takes more than 10 minutes move it to in progress and fix it yourself
  • So it will go through code review again after that
• The pair doing the code review makes the final decision or we have a team meeting about it *

Keep in mind that it's OK if someone didn't solve the problem the same exact way you would have done it.

These were some ideas we brainstormed during a retrospective on what to add to a code review checklist. The items with an asterisk at the end are what the team agreed upon.