Thanks for sharing. It's a good way to think about code review - as taking the best from each other and learning together.
Particularly the bit about "Is style important" resonated with me ("The study... found... people who received a lot of style comments on their code, reviewed those reviewers rather negatively") - I feel much happier ever since we discussed and introduced style guides e.g. jscs in many of our projects and it's a reminder when we notice pain in code review, to think of ways to improve the way we work to minimise these.
Personally, I will be more mindful about asking questions more than demanding for changes, to help build a better culture in our team.
On Wed, Jul 1, 2015 at 9:57 PM, Stephen Niedzielski sniedzielski@wikimedia.org wrote:
What a great recommendation! Thanks for sharing!
--stephen
On Mon, Jun 29, 2015 at 10:51 AM, Toby Negrin tnegrin@wikimedia.org wrote:
Thanks for posting this Sam -- code review is one of the best parts of our engineering culture, I'll definitely watch this when I get a chance.
-Toby
On Mon, Jun 29, 2015 at 7:59 AM, Sam Smith samsmith@wikimedia.org wrote:
Hey y'all,
I watch a lot of talks in my downtime. I even post the ones I like to a Tumblr… sometimes [0]. I felt like sharing Derek Prior's "Implementing a Strong Code Review Culture" from RailsConf 2015 in particular because it's relevant to the conversations that the Reading Web team are having around process and quality. You can watch the talk on YouTube [1] and, if you're keen, you can read the paper that's referenced over at Microsoft Research [2].
I particularly like the challenge of providing two paragraphs of context in a commit message – to introduce the problem and your solution – and trying to overcome negativity bias in written communication* by offering compliments whenever possible and asking, not telling, while providing critical feedback.
I hope you enjoy the talk as much as I did.
–Sam
[0] http://sometalks.tumblr.com/ [1] https://www.youtube.com/watch?v=PJjmw9TRB7s [2] http://research.microsoft.com/apps/pubs/default.aspx?id=180283
- The speaker said "research has shown" but I didn't see a citation
Notes (width added emphasis)
Code review isn't for catching bugs "Expectations, Outcomes, and Challenges of Modern Code Review" Chief benefits of code review:
Knowledge transfer Increased team awareness Finding alternative solutions
Code review is "the discipline of explaining your code to your peers" Process is more important than the result Goes on to define code review as "the discipline of discussing your code with your peers" If we get better at code review, then we'll get better at communicating technically as a team
Rules of Engagement
As an author, provide context
"If content is king, then context is God" In a pull request (patch set) the code is the content and the commit message is the context Provide sufficient context - bring the reviewer up to speed with what you've been doing in the past X hours Challenge: provide at least two paragraphs of context in your commit message This additional context lives on in the commit history whereas links to issue trackers might not
As a reviewer, ask questions rather than making demands
Research has shown that there's a negativity bias in written communication. Offer compliments whenever you can When you need to provide critical feedback, ask, don't tell, e.g. "extract a service to reduce some of this duplication" could be formulated as "what do you think about extracting a service to reduce some of this duplication?"
"Did you consider?", "can you clarify?" "Why didn't you just..." is framed negatively and includes the word just
Use the Socratic method: asking and answering questions to stimulate critical thinking and to illuminate ideas
Insist on high quality reviews, but agree to disagree
Conflict is good. Conflict drives a higher standard of coding provided there's healthy debate Everyone has a minimum bar to entry for quality. Once that bar is met, then everything else is a trade-off Reasonable people disagree all the time Review what's important to you SRP (Single Responsibility Principle) (the S from SOLID)
Naming Complexity Test Coverage ... (whatever else you're comfortable in giving feedback on)
What about style?
Style is important "People who received style comments on their code perceived that review negatively" Adopt a styleguide
Benefits of a Strong Code Review Culture
Better code Better developers through constant knowledge transfer Team ownership of code, which leads to fewer silos Healthy debate
Mobile-l mailing list Mobile-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mobile-l
Mobile-l mailing list Mobile-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mobile-l
Mobile-l mailing list Mobile-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mobile-l