Working with some terrific developers at Kajima for the last few months has helped me to learn loads; it’s been great working in a big Rails team for the first time since moving to Ruby from PHP back in 2013, and one of the biggest changes to working alone on projects is having a peer review process for code before it gets merged to master.
As a developer, you always have this imaginary monkey on your shoulder anyway, whispering to you every time you cut corners. “That’s not what you’re supposed to do…”
Working in a team, that imaginary monkey is replaced by real, warm-blooded human beings and the whispering is replaced by written comments on Github pull requests – suddenly you’re held to account for your decisions and working to meet a commonly agreed bar for what good, acceptable code looks like.
One of my colleagues shared this great talk from Railsconf 2015, well worth watching:
Here are my notes:
We think that code review is for catching bugs – but really it’s not. Few bugs are caught this way, but we are all under the impression it catches more. Nope!
It’s for helping us to get better every day.
Of course, a few bugs get caught, but the real benefits include:
-
Knowledge transfer
-
Increased team awareness
-
Finding alternative solutions
The discipline of discussing code with your peers – the process is more important than the net outcome of the results or changes in the code
Improving the technical communication on your team
Strong code review culture isn’t seniors explaining things to juniors, it has to be a real conversation
Some rules from Thoughtbot
Two key things
1. As an author…provide sufficient context
-
“If content is king then context is god”? Content is the diff. Context is WHY that changed. What’s the problem your changes solve? Insufficient context is one of the chief impediments to good reviewing. So… commit messages that say WHAT are no good, must explain WHY these lines changed or were added. A link to Jira is not an explanation! Don’t make the reviewer hunt for this context. Also, the thing on the end of the link probably doesn’t explain the context for the change directly.
-
Challenge – at least two paragraphs of context for every change you make!
2. As a reviewer… ask questions rather than making demands
-
ask, don’t tell
-
Negativity bias in written communication compared to verbal. Inflection compensates far more than we realise, the absence needs to be supplied in writing. Saying the same words in writing that you’d say will be interpreted harshly or critically.
-
Engage in conversation, don’t dictate what to do. Avoid imperatives
-
Leads to ignoring or arguing (ignoring is worse). Give the author credit for having considered your suggestion and assume best intentions. Frame it as a question! “What about xxxx?”
-
Aim is to foster technical discussion
-
“What do you think about…?”
-
“Did you consider…?”
-
“Can you clarify…?”
Imperatives are particularly harmful when difference in level – juniors will ALWAYS do what seniors tell them but rarely understand why
AVOID
-
Why didn’t you just …? (Just is a bad word)
-
Why didn’t you… ? Still puts on defensive, frames negatively (prefer “How about” instead of “why not”)
Socratic method – ask questions to stimulate critical thinking and illuminate potential alternatives
How to handle conflict?
Conflict is good, healthy debate drives quality and leads to learning. But two types
Good – we don’t agree on an issue
Once you reach a minimum bar of quality, we’re now talking about tradeoffs. “Not the way I’d do it” is not in itself a reason to change things.
What if we don’t agree on the process (committing to master, ignoring feedback etc) Get ahead of this by establishing the social contract that everyone is agreed with
You can review any code any time, just because it got committed doesn’t mean there’s no value to reviewing it.
What to review – if I’m not catching bugs, what am I doing this for?
Timing – ten minutes is a long time to spend on a review. Small changes are easier to review
-
Single responsibility principle – does every object have just one job
-
Naming things
-
Complexity
-
Test coverage
-
Web security
-
Style (but style nitpicks lead to reviewers being perceived negatively!)
But it’s fine for people to review in their own way, bringing their own strengths
“So and so approved it” not doing QA
Adopt a style guide to remove potential for pointless arguments and use Rubocop. If all code passes linting then there will be a lot fewer style comments.
Insist on high quality reviews, but agree to disagree.
Review what’s important to you
Aim to have a strong code review culture – valuable in itself.
Benefits
-
Better code
-
Better developers
-
Team ownership – this is _our_ code
-
Healthy debate (not silent seething)
The fact reviews are asynchronous may be a benefit; means the PR and diffs have to stand on their own.
