Engineering
  • Welcome!
  • First Principles
  • Communication tools
  • Acronyms
  • Vital reading material
  • Engineering Onboarding
    • Onboarding Guide
    • Mobius onboarding Guide
    • Lessons
      • 001 - Function Purity
      • WIP - Shared Mutable State
  • System design & Architecture
    • Introduction
    • Refactoring
    • Architecture Decision Records
  • Meetings
    • Iteration Planning Meetings
    • Daily Standup Meetings
    • Retro meetings (retrospective)
  • Release Engineering
    • Introduction
    • Git commit messages
    • Code reviews & pull requests
    • Trunk-based development
      • Our methodology
    • Service Dependencies
    • Tooling & infrastructure
    • Git commands & workflows
  • Project Management
    • Product Requirements Document
    • Pivotal Tracker
Powered by GitBook
On this page
  • Most important points
  • Assigning a PR
  • Helpful tips
  • More reading material

Was this helpful?

  1. Release Engineering

Code reviews & pull requests

Regular, team-wide code reviews are important for the health of the software being built.

PreviousGit commit messagesNextTrunk-based development

Last updated 5 years ago

Was this helpful?

Code reviews ensure standards and guidelines are respected and followed, and the larger team learns about the new code being written. Sure, code reviews can (and do!) help with catching mistakes early on, but in large codebases, techniques like test-driven-development are far more effective for writing bug-free code.

Code reviews are an opportunity for discussion and evaluation; a chance for novice engineers to get feedback on their ideas from senior ones, and a time for senior engineers to walk the rest of the team through their thought process.

Most important points

It is the code that is being reviewed, and not the person who wrote it!

  • Many programming decisions are opinions and not facts. When discussing tradeoffs, ensure it doesn’t lead to bike-shedding and a cost-benefit analysis is done -- “What is the cost of discussing this thing for 60 minutes? What is the benefit or drawback of picking a choice? Is it worth our time?”

  • Ask questions and don’t demand changes. Good questions avoid judgment and avoid assumptions about the author's perspective.

  • Don’t use hyperbolic words and avoid all sarcasm. Temper your communication, sit down, stay humble.

Avoid selective ownership of code. Eventually, it is the team’s code and the entire team has to share the responsibility of maintenance. Team and project standards are much more important than personal opinions. When disagreeing, first commit to using the prevailing standard. Discuss the standard later... right here, in the handbook! Just .

Assigning a PR

You can automate this using !

  • In a round-robin fashion, assign your PR to a team member. This should be done without asking them. Everyone should be coding and reviewing at all times.

  • If you think your PR is non-trivial and needs more than one pair of eyes, then assign it to two people. If you’re new to the team, assign it to two people.

  • If your PR isn’t reviewed in the next 24 hours, talk to the assignee in person and figure out what’s happening. Maybe they're busier than usual and need a helping hand?

Helpful tips

  • It is often easier to follow the author's train of thought by reviewing a PR commit by commit, instead of reading the entire diff file by file.

  • When commenting on an intermediate commit in a PR, one should first make sure that the comment makes sense when the complete PR is seen as a whole. Because the author may have already addressed the concern in another commit down the PR.

More reading material

If you're reviewing on github.com itself, there are you can use!

, on the Google engineering practices website.

open a pull request
a tool like PullAssigner
handy keyboard shortcuts
How to do a code review