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:
-
Knowledge transfer
-
Increased team awareness
-
Finding alternative solutions
-
“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!
-
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…?”
-
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”)
-
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!)
-
Better code
-
Better developers
-
Team ownership – this is _our_ code
-
Healthy debate (not silent seething)