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
<sniedziel...@wikimedia.org> wrote:
> What a great recommendation! Thanks for sharing!
>
>
> --stephen
>
> On Mon, Jun 29, 2015 at 10:51 AM, Toby Negrin <tneg...@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 <samsm...@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
>
_______________________________________________
Mobile-l mailing list
Mobile-l@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mobile-l
