Skip to content

Instantly share code, notes, and snippets.

@booch
Last active November 21, 2017 12:11
Show Gist options
  • Star 1 You must be signed in to star a gist
  • Fork 1 You must be signed in to fork a gist
  • Save booch/be2b27447ba489eccce5 to your computer and use it in GitHub Desktop.
Save booch/be2b27447ba489eccce5 to your computer and use it in GitHub Desktop.
Some guidelines for doing a code review

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.

@booch
Copy link
Author

booch commented Oct 31, 2014

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment