Skip to content

Code Review Tips

Jump to bottom
shunter edited this page Feb 14, 2013 · 12 revisions

All of our code is peer reviewed before merging a pull request into master. We review code to share knowledge; foster shared ownership; and improve code quality and consistency.

Tips

  • GitHub's code review tools are awesome.
  • Delete the branch after merging the pull request:

  • Just knowing that our code is going to be reviewed helps improve its quality and our rigor, e.g., we don't skimp on tests and doc.
  • Anyone is encouraged to review anything that interests them. However, someone familiar with the changed code should ultimately merge the pull request.
  • Some changes can be reviewed by reading the code alone; others require running the code, examples, and tests. Don't forget to check for JSHint warnings, either through Eclipse or by running the jsHint Ant task.
  • Most pull requests require additional commits, often minor but sometimes major, before being merged.
  • Comments are about code, not the person who wrote the code. Don't be offended by a reviewer's comments and don't aim to offend when commenting. We all have the same vision and want the same thing - to be awesome.
  • Be concise. Make every word tell. Less reading means more coding.
  • Provide motivation. Unless it is obvious, don't just suggest a change. Suggest why the change should be made.
  • See the forest through the trees. Don't just review code one line at a time. Consider the big picture and its implications.
  • Bring other people into the conversation. If someone has expertise with a particular language feature or problem domain under review, invite them to help.
  • Limit the scope. As a reviewer, it is easy to want to increase the scope of a pull request - why don't we do this everywhere?. These are often fair questions but can be better served by submitting a separate issue to allow more incremental pull requests.
  • Keep pace. The developer should expect prompt feedback from reviewers, and reviewers should expect the same. If not, politely ask for it. We all want pull requests to get into master.

A pull request is a sincere request for feedback, and those reviewing the request work with the developer to get the code merged.

Resources

Clone this wiki locally