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
