This guide exists to help spreading good code review practices in teams.
It aims to be a reference to show to your team, along with some good examples.
- Intro
- Anatomy of a PR
- How to review other people's code
- How to receive criticism and be more awesome
- Tips and tricks
- Good examples
- Bad examples
- Glossary
- Good resources
This guide is best suited for companies/teams using github or github entreprise to manage their code and code reviews, but the same principles apply elsewhere.
It will try to explain the best practices on creating PRs, reviewing them, common ways of doing things, the dos and don'ts, and we will cover the 
squirrel.
Inspired by this post on github blog and the thoughtbot guide.
PR is a github-specific concept which means pull request. You can read on the official docs with plenty of screenshots, but essentially,
PR is an UI for merging changes from one branch to another, that allows discussion, pasting silly gifs, commenting on particular lines, and by doing that, documenting the change and the thinking behind for later (think onboarding a new person, or just refreshing your own memory).
The scope of a PR will depend vastly on the actual changes that you are implementing, the project etc, but here are some guidelines:
- bundles together changes which are truly connected
- says exactly what it does on the tin
- contains enough info for someone new to pick it up later and understand what's going on
- contains a healthy dose of self-distance
- makes use of the all the goodies available (lists, screenshots, links, etc).
- has no description and/or uninformative title
- does more or less than it says on the tin
- handles issues which are unrelated
PR s are made by taking a branch, that you want to merge into another one. So for a good PR, you need a good branch discipline.
This is going to depend on the dynamics between the team members, but again, here's some stuff that tends to work well:
- give the branch a reasonable name. Some common practices include adding prefixes, for example:
fix/stale-connections-to-dbfeature/add-pagingexperiment/reimplement-proxy-in-go- remove the branch after the PR is merged in (click the button at the bottom of the PR)
- commit the changes in small, easily reversible increments
- apply logic and behave your best
If you're not 100% confident with git branches, make sure you check out this awesome tutorial.
Descriptions are written in Markdown, so they support all the goodies, like lists, code blocks, quotes, links, inline images, emojis, headers, etc. Use them.
A good description is one that you can dig out 6 months later, read, and:
- understand why the change was needed,
- see clearly what were the difficulties,
- understand what decisions were made and why,
- and know how to verify that it works.
With that goal in mind, it will be tough to go wrong.
Please make sure you also read this post from github people.
Reviewing other people's code doesn't have to be boring, and the review shouldn't take more than 30 minutes (including checking out the code, running the tests and playing around). If it does, your PR is probably too big.
Don't think I'm a nerd, but with a good use of gifs and the  inline image tag, they can be really fun.
When reviewing someone else's code, you are in the position of power. They did their work, their code is now under a microscope, and all the weaknesses are now going under a hammer. Be careful to not use that position to be overly negative. It will be your turn soon, so treat people the way you'd like to be treated. Don't make enemies.
- don't be a dick. You're looking at someone's work, and you should assume they are capable and well-meaning. If they are a weaker developer than you are, wait until your code gets under a microscope of someone stronger than you.
- be humble - see point 1. The more you know, the more humble you get.
- help people grow. You are here to help them avoid mistakes in the future and improve the overall quality of the product. Don't prove you're smarter or know more. Lead by example.
- realise that most things are up for discussion and few are absolute truths. So don't present opinions as facts
supertest is better. UseI like supertest more, because it gives us A, B and C essentially for free. - suggest, don't demand.
What do you think of using Mock class here ?always beatsuse Mock class.I'm wondering,I think,Wouldn't it beare your best allies. - use gifs and memes as long as that's your thing. They really have the potential to make everybody chill.
- remember that things get often personal, because the work produces is seen as a direct product of a person's capabilities. So, again, suggest, don't criticise.
- when clarifying, use links to other repos, to a specific line. Cuts the communication required down very nicely.
- talk in real life. But after you do, put a comment with a summary of what you decided on.
- be careful with sarcasm. It goes well with some people, but often sounds much more harsh than intended, when read in isolation.
- use code snippets to be explicit, but not show off, when applicable.
You might have noticed, that the first three points are very similar. I wonder what that might mean ?
In this situation, all of the little blibs and non-optimal solutions will be resurfacing, with your name attached to it. At the beginning it sounds little bit stressful, but here's a couple of guidelines to make it more manageable and fun:
- do your homework by prepping the PR properly (description, how to test, what's important to look at). Imagine a Michelin star restaurant meal vs a stale hamburger. No one flocks to dealing with your stale hamburger.
- don't hide in the corner. Hiding dodgy bits in a big PR is not a working technique. Unless your goal is to enrage your colleagues.
- don't take things too personally. The code review process is there to catch all the mistakes and shady shortcuts, so dont' be surprised/embarassed/upset, when it does its job. The review is of a piece of code, and that piece only, not your capacities as a person.
- learn from each review. Many people fall into a trap of thinking that they can't learn from a person below them on the paygrade. I personally think that everybody can teach me something, and it has always been true for me.
- be grateful for the review. Someone's essentially giving out the info that makes them a stronger dev than you are, and you are free to take it and we as awesome the next time. How cool is that ?
- be responsive. A stale PR is like an old piece of pizza - still better than nothing, probably edible, but would have been much better two days ago.
- invite people to review your PR. It takes time to do it, so encourage them to do it for you. Don't dread being wrong - that's the best way to be right the next time.
You can specify which issues should be closes when merging. To do that, just say in the description Closes #12 or Closes http://github.com/seeker89/code-review-PR-quide/issues/1.
The syntax is the following: 
This is how you do that (specify the syntax after the triple "`" sign):
def time_to_roll():
"""Prints the essence of life""
print("https://i.giphy.com/kFgzrTt798d2w.gif")Just kidding 😃. We won't be digging them out from the lurky search engine results... Or will we ?
- [WIP] (also WIP, WiP) - Work in Progress. Means, that the PR is not yet in its final form, but it's already been created to track comments, links, and get opinions. Often a better alternative to trying to keep a branch private, until you consider it ready to ship, because early suggestions might prevent mistakes.
- ,
:shipit:, squirrel - a sign, that one person sees the PR fit of merging in. The story about why that's a squirrel is long. Alternatives include 🚢🇮🇹 and 🐑🇮🇹. - LGTM - Let's Get That Money - OK, let's ship it. Usually used, when no real review is required, because the change is small and obvious.
- RFC - Request For Comments (not github specific, but useful). Used when a draft of a design document is ready, and we talk on a much higher, abstract level of the idea described, more than the contents of the PR.
- TL;DR - Too Long; Didn't Read - most often used to introduce a very short description of the essence of the text that follows.