Why code review?

• Ideally everyone on the team should be familiar with all of the code (even what they aren't working on) in case the original author is busy or unavailable (or has graduated). Reviewing code improves your familiarity with the codebase, and gives you insight into why design decisions were made.
• Code is read more often than it is written. Code reviews encourage everyone to write readable (and maintainable) code
• Constructive feedback makes us better programmers
• Provides visibility into the progress of each project
• To catch bugs
• It forces the author to reason about their code

Responsibilities as a PR Author

As an Author, you've written some dank-ass code that you want merged into the repository.

Responsibilities

Testing

Test your code, and provide your reviewers a way to see the results of your testing. Either write unit tests or provide a trace or scope capture or something..

Don't open a pull request with code that wouldn't pass your own review standards.

Small Incremental Changes

There's probably studies that show this, but personally I like to keep changes relatively small. Typically I aim under 1,000 lines per diff. If you go over due to unit tests, that's not too big of a deal.

On one hand, if you're making changes to a lot of files, frequently merging into mainline means you rarely have to deal with merge conflicts, and when you do, it's significantly less painful to deal with. In addition. this forces you to think of features as incremental changes instead of dropping the entire change set all at once. I find this generally helps with modularization as well, and allows you to better scope out your work.

Linter/Formatter

Run the linter and formatter over your code before submitting a pull request. This ensures that you let a computer catch all the simple style guide violations before asking for the team's feedback.

But why have a style guide in the first place? When other people read the code (usually when fixing a bug), it helps to keep the code style as consistent as possible such that there isn't additional cognitive load to parse your style.

Having a linter + formatter also reduces the amount of noise that a reviewer has to point out during the pull request. If you can deal with all the small things so that reviewers can focus on the "things that matter", that's beneficial for everyone. This is why we try to have an opinionated linter, and have auto formatters such that the build fails when it doesn't meet the style guide.

Other

• Code review is a collaborative feedback process! If you disagree with a reviewer’s comment, you should start a conversation about it, don't just ignore it, or implement it even though you disagree.

A PR Reviewer

As a reviewer, you've been asked to review someone else's code.

Responsibilities

It's easy to focus on Grammar and the tiny details (like pointing out spelling mistakes), while I think that it's more beneficial to start at Intent and work your way down. The reason for this is that if you begin giving feedback on variable names, and other small details (which are the easiest to notice), you're going to be less likely to notice that the entire patch is in the wrong place! Or that you didn't want the patch in the first place!

Doing reviews on concepts and architecture is HARD. It's much harder than reviewing individual lines of code, so I think it's important to force yourself to start there.

Intent

• What change is the PR author trying to make?
• Is the bug they're fixing really a bug?
• Is the feature they're adding one we want?
• Is this the approach we want to take?

Structure

• Are they making the change in the right place?
• Is this a hacky workaround for a larger underlying problem?
• How is their code structured?

Implementation

This is the nitty-gritty of code review:

• Does the diff do what it says?
• Is it possibly introducing new bugs?
• Does it have documentation and tests?

Grammar

• Does this variable need a better name?
• Should that be a keyword argument?
• Typos?

Tips

• As a reviewer, you're not just hunting for mistakes! I'm also guilty of this, but you should absolutely comment on things that are done well! Maybe they've brought up a case you didn't think about, or wrote some slick code in a way you didn't think of.
• Don't just say "this is wrong" or point out a mistake, suggest how it can be done better

Mindset

Hopefully as Authors and Reviewers we can make the positive column a reality.

Author

Reviewer