Code Review Etiquette

Can you give better feedback?

Photo by You X Ventures on Unsplash

Look — we’ve all been through it.

You pick up a Pull Request and see that there are 1500 lines that have changed. Nobody reviews the PR. This is a failure since:

The main purpose of a PR is to initiate code review

Yes there are pleasant side-effects of the process, avoiding rollbacks and production issues (hello, hard to find bugs).

The poor behaviour to avoid

The fact is that you don’t want to be this guy:

If you don’t care, why should I?

or

If I don’t care, why should you?

This gives us a set of behaviours and actions that you want to avoid. Here are a choice few

• The PR has no description, not giving a clue about what the PR is trying to fix
• The PR topic is generated from GIT based on the branch name (that is: the default!)
• The PR is an Epic commit with multiple goals from refactoring to a feature
• The commit history is madness, with unhelpful commit messages

So what should we do to try to keep your teammates happy?

The good behaviour to aim for

Overall PRs are there to make a better product by all involved. Buy into that and the rest becomes self-evident.

• Use good commit messages. This enables your review to check the PR commit by commit and understand your thinking — and all test should pass for each commit (go green!)
• Commits should be self-contained and make sense standing on their own
• If you’ve made a mistake use Git to rewrite history
• Create a PR description that gives the reviewer a good understanding of why the code has changed. If you’ve changed the…