Pull Request Unicorn

This is an adaptation of a talk I gave at at previous workplace. It's not a proscription - it's just what worked for one team at one particular time. The most important thing is to communicate with your team and set expectations.

Objectives

We all do code review but some of us don't really know what the process is for and what we're trying to get out of it. What is this ceremony for?

The Right Thing

The first responsibility of the team is to make sure that they deliver the thing they actually agreed to deliver.

This is most often forgotten reason for doing code review. Developers are in the habit of writing some code and opening a PR as a single atomic process. They don't even really stop to think about it. Ask yourself; does this thing actually do what we agreed to do? If not why?

Down with this sort of thing

We want to reduce the defect rate of code that makes it into production. We know that we can't eliminate all bugs (at least not without truly heroic effort) but we can minimise them by getting several people to think about the code being written and shipped.

Up with this sort of thing

We want to make sure that the code we merge to master adheres to good design principals as they're understood by the team at the time. What do we mean by good design?

• Loose coupling
• Low cyclomatic complexity
• Speaks in the language of the business
• Whatever else your team thinks is good

Knowledge transfer

We want to provide a learning opportunity for everyone involved. This is why we ask everyone to be involved and not just "seniors" acting as gateways. Don't be afraid to review someone else's code some of the best code review comments I've ever gotten are from less experienced team members asking me to explain my choices.

Principals

Whether we're performing a code review or requesting one there are a few principals we should adhere to:

Assume good faith

Everyone is doing the best that they can with the skills and knowledge that they have. If someone has done something wrong it's not because they're dumb or malicious.

Collaboration

One person may have written the code, but the team is responsible for delivering it. That means we all have to work together and own the process from when the first line of code is written (or hopefully deleted!) right through to when it's shipped to production.

Kindness

Have you talked with your team about nonviolent communication? If not it's a great idea to book a workshop for some professional development.

When giving feedback try to use "I" statements rather than "you" statements.

When you're ready to give feedback make sure that it is Actionable, Specific and Kind (ASK). This means:

• Actionable - the receiver is able to understand what is is you want them to do.
• Specific - the feedback should be specific enough that the receiver can come up with concrete steps to achieve the outcome.
• Kind - this should be obvious but so many people forget it when they're giving feedback.

Participation

All team members should regularly take part in code reviews regardless of their level of experience. It's okay to not understand something - it's often a sign that something can be improved.

Unsurprisingness

Boring code is good code. The first person you outsmart with clever code is your future self. Resist the urge.

Asking for review

With those principals and objectives in mind lets make sure that you're the best pull requester you can be.

Let’s start by learning how we know we’re ready to open a pull request. What needs to happen before you open that PR?

Acceptance Criteria

As the author you believe that the feature meets or exceeds the acceptance criteria agreed to with the product owner either as set out in the story/task definition or in subsequent negotiation with them over the duration of development.

Eco-friendly

Lots of green dots.

Better than just meeting the acceptance criteria there needs to be acceptance tests which prove that you've met them. Also there should be adequate unit tests in place for new code or components.

Bad cop

There shouldn't be any linting or formatting errors in your code or any related artefacts. It's not a good use of anyone's time to put comments on your PR asking you to fix the indenting of an HTML div or put brackets around your method calls.

Debuggered

There are no debugger, todos, fixmes or other random garbage comments in your code. In fact no commented out code at all. We have version control - we can get it back if we need it.

Teh

There aren’t any obvious typos or misspellings in the code.

TLDR; read the diff before you hit submit.

Subject vs object

Let’s talk about writing your PR, starting with the subject. Whenever I look at my bitbucket dashboard I see a list of PRs by subject. That subject line better be enough to give me context about what the feature is and whether it needs my attention.

Good

[FRT-675] Enable customers to pay their bills via stripe.
[Card 27] Locked out users should be able to unlock their account.
[BUG] Fix 500 error when logging in.

Bad

Feature/widgets-controller
oops
make the PO happy

A healthy body

Obviously, your catchy subject has caught my eye and I’m intrigued enough to keep reading. I need to learn enough about the feature in order to be able to judge the content of your request:

Resist the easy option

I know that bitbucket "helpfully" gives you a list of your branch's commit messages when you create a new pull request. I really don't want to see them. Please for the love of all that is good in the world.

Tell a story

I need to know the story that you’re trying to implement.
You don’t need to paste the exact story, but I need to get an understanding of what the story is that you’ve agreed to build for the product owner.
I also need to know some of the reasoning for the story, so that I can make better suggestions.

Negotiations

I need to know about any conversations that may have changed the scope of the feature while you were building it.

• Did you talk to the product owner and add or remove things?
• Did you talk it over with your team and decide to do something different?

Link me up, Scotty

If I have access to read the story directly then please make sure that there’s a link to the original story in the PR, that way I can see for myself if I have additional questions which are unanswered. If not it may be a good idea to add a summary of the discussion that took place.

A picture is worth a thousand words

If you've made or changed some UI then it's always a good idea to add before and after screenshots so that I don't have to check out your branch, install the dependencies and fire up the application just to see what it looks like.

A video is worth a thousand pictures

If you've added UI behaviour or interactions then it's always a good idea to add a short screen capture of it either as a video or an animated GIF.

I use LICEcap because it screen records straight to GIF files and pretty much nothing else.

Merging

When should you merge your change? Simple: when you have two ticks. After that it's at your discretion. There may be project constraints on when things can be merged or you may wish to add your own constraints. For instance I often will not merge my approved changes if there is no feedback because I fear that less experienced developers may be just rubber-stamping my work.

Performing a review

Let's flip this over and talk about reviewing someone else's code. There's a few things you need to know:

Targeted feedback

Try and put your comments as close as possible to the code your commenting on - on the line you want changed is best.

Why should they care?

Do your best to explain why you're suggesting this change. Is it readability, badly designed or just flat out wrong?

What are the trade-offs of your suggested approach?

Give an example of how you think it can be improved.

Hugs not thugs

Remember to ask questions like "have you considered" or "I feel like" rather than "that's wrong" or "you're dumb".

Speaking of dumb...

Ask dumb questions. Especially if you're not very experienced. If you don't understand then chances are the author doesn't either.

The non-blocking elephant

Sometimes you may want to make a suggestion or point out something without the author being forced to address it as part of their change. Perhaps there's something interesting you thought of while reviewing their code or there's a teachable moment. Our team decided to use the 🐘 emoji as a way to symbolise this.

Nice work old chap

Remember to give praise. When someone does a good job or does something that delights you drop ✨, 💖, 🦄 or some other amazing emoji on their PR. It'll make then feel better and is just a legit way to be nice.

Celebrate

Have an approval party. Go over and give them a high five or fill their lunch box with glitter. Whatever works within your team's culture.

Thanks

Thanks for reading! Let's make code review great again!