Code Review
A guide for reviewing code and having your code reviewed. As part of our Pull Request process, this’ll happen often.
Who Does Code Reviews
We all review each other, regardless of previous experience or time at Boltmade. This has two extra benefits:
- It evens the playing field, and helps everyone feel like they are on the same page.
- Experienced developers in the language will be challenged to reaffirm their assumptions when questioned on things they assume and take for granted.
Giving & Receiving Effective Feedback
Since the goal of pull requests is to help people improve and learn from each other, there are some simple rules we can follow to make this more efficient:
- Realize nobody is perfect, neither reviewer nor reviewee.
- Accept that many programming decisions are opinions. Discuss tradeoffs, which you prefer, and reach a resolution quickly.
- Ask questions; don’t make demands. (“What do you think about naming this
links_for_category?”) - Ask for clarification. (“Is this supposed to do X?” or point out “I’m confused, it seems like this is supposed to do X.”)
- Avoid selective ownership of code. (“mine”, “not mine”, “yours”)
- Avoid using terms that could be seen as referring to personal traits. (“dumb”, “stupid”). Assume everyone is attractive, intelligent, and well-meaning.
- Be explicit. Remember people don’t always understand your intentions online.
- Be humble. (“I’m not sure - let’s look it up.”)
- Don’t use hyperbole. (“always”, “never”, “endlessly”, “nothing”)
- Don’t use sarcasm.
- Keep it real. If emoji, animated gifs, or humor aren’t you, don’t force them. If they are, use them liberally.
- Don’t be scared to go talk in person. If there are too many “I didn’t understand”, “Alternative solution:” comments, or discussion is getting intense; go say hi. Post a follow-up comment summarizing offline discussion.
Keep in mind: it’s extremely easy to write code that a computer understands. Martin Fowler put it best in Refactoring: Improving the Design of Existing Code:
“Any fool can write code that a computer can understand. Good programmers write code that humans can understand.”
Having Your Code Reviewed
- Be grateful for the reviewer’s suggestions. (“Good call. I’ll make that change.”)
- Don’t take it personally. The review is of the code, not you.
- Explain why the code exists. (“It’s like that because of these reasons. Would it be more clear if I rename this class/file/method/variable?”)
- Extract some changes and refactorings into future tickets/stories.
- Push commits based on earlier rounds of feedback as isolated commits to the branch. Do not squash until the branch is ready to merge. Reviewers should be able to read individual updates based on their earlier feedback.
- Seek to understand the reviewer’s perspective.
- Try to respond to every comment.
- Wait to merge the branch until Continuous Integration (TDDium, TravisCI, etc.) tells you the test suite is green in the branch.
- Merge once you feel confident in the code and its impact on the project.
Reviewing Code Effectively
Understand why the code is necessary (bug, user experience, refactoring). Then:
- Communicate which ideas you feel strongly about and those you don’t.
- Identify ways to simplify the code while still solving the problem.
- If discussions turn too philosophical or academic, move the discussion offline.
- Offer alternative implementations, but assume the author already considered them. (“What do you think about using a custom validator here?”)
- Seek to understand the author’s perspective.
- Sign off on the pull request with a gif, your favourite emoji, or a “Ready to merge” comment.
What If We Can’t Agree
Every now and then, two people will come to a standstill. At this point it’s important to take a step back and realize that the real goal is not to be right, but to find the best solution.
If the argument is over something arbitrary, ask another Boltmadian to decide.
Otherwise, the almost universal solution is to go chat in person. Ask “Did I offend you?” Talk. This often leads to both parties realizing that there has been a miscommunication. Remember, text-based discussions lose much of the subtleties of communication, which is why going to chat in person can be so effective. You will often find out that while you thought the reviewer was being harsh, it was actually a miscommunication. These things happen.
In order to avoid situations like this, always remember that we review the code, not the other person. Criticism should be taken professionally and not personally.