how to successfully grow a code review culture
TRANSCRIPT
How to successfully grow a code review culture
@nnja Nina Zakharenko
What will we learn today? - Part 1
Why do we code review? What are the benefits? Our Code Review Process How to Grow a Culture
What will we learn today? - Part 2
How to be a Great Reviewer How to a be a Great Submitter Using Code Review to Build a Stronger Team
Not one size fits all!
team size 2 vs 10
product type agency vs in-house
defect tolerance jet engines vs mobile games
Why Code Review?
?
Code Reviews Can Be Frustrating
Apparent Code Review Frustrations
Adds time demand Adds process Can bring up team tensions “smart” devs think they don’t need it
How do you get people to change the way they work?
Show them the Benefits are Crystal Clear
Find Design Flaws & Bugs
So they can be identified and remedied before the code is complete Case Studies on Review:
⬇ bug rate by 80%
⬆ productivity by 15%
https://blog.codinghorror.com/code-reviews-just-do-it/
The goal is to find bugs before your customers do.
Shared OwnershipWe’re all in this together
👀 results in positive motivation Overall increase in code quality
Shared Knowledge No developer is the only expert Developers gain familiarity with different modules via review Decrease the “Bus Factor”
🚍 Factor? concentrated specialized knowledge will one person sabotage the project if they left?
Leave a Paper Trail Code Reviews add a wealth of knowledge! Why was a particular decision made?
Find Bugs Shared Ownership Shared Knowledge Reduce Bus Factor Leave a Paper Trail
Code Review Benefits?
My current organization has a great review culture.
As a result, I’ve become a better programmer.
What’s Our Process?
Tools We use GitHub enterprise Many other tools exist GitHub added Review tools September 2016!
Formal Reviews
Inline Comments
Blocked Merging
Workflow
make feature branch
do workopen pull request
Workflow
assign reviewer
complete review
merge branch upstream
Feature Branches? Feature branches are merged into a development branch, not master.
We can QA in a staging environment that mimics production When necessary, we can hotfix master
Grow a Culture
It starts with a Commitment
Make a commitment to always Review before Merge Present the Facts Don’t force it New ideas take time to root
Code Reviews need to be Universal
Doesn’t matter how Senior / Junior you are Only Senior Devs reviewing == bottleneck Inequality breeds dissatisfaction Break down old barriers and status quos
Code Review is done by your peers,
not management.
Consistent Code Your code isn’t yours, it belongs to your Company Code should fit your company’s expectations and style, not your own. Reviews should encourage consistency for code longevity
Style Guide Distinguishes personal taste from functionality Should be agreed upon before hand Great codebases are written by a team, but look like they were written by an individual
Consistent Code is Easier to Maintain by a Team
No Blame Culture Failure is inevitable Mistakes become team, not individual responsibility Needs management support
Blameless Post Mortems Meet shortly after Incident Resolved Understand the root cause Document how it was fixed Identify how to prevent it in future
For a blameless culture, don’t point fingers!
“
When code reviews are part of the culture, people don’t expect their
changes to be reviewed, they want their changes to be reviewed.
https://www.caktusgroup.com/blog/2014/07/28/culture-code-reviews/
Make a Commitment Universal Code Review Performed by Peers Style Guide = Consistency
How to Grow Culture?
Starts with No Blame Culture
How to Grow Culture?
How should we code review?
?
How to be a Great Reviewer
Have EmpathyWhy do you think you’re so hostile in code reviews?
If only I had been more popular in
High School.
Be Objective“this method is missing a docstring”
instead of “you forgot to write a docstring”
Ask Questions Don’t Give Answers
“Would it make more sense if… ?” “Did you think about… ? ”
Offer Suggestions “it might be easier to” “we tend to do it this way”
Avoid these terms Simply Easily Just Obviously Well, actually…
… now, simply
Have Clear Feedback Strongly support your opinions Share How & Why Link to supporting blogs, stackoverflow examples, or documentation.
Not clear feedback
Compliment Good Work & Great Ideas
Don’t be a perfectionist
Don’t be a Perfectionist For big issues, don’t let perfect get in the way of perfectly acceptable. Prioritize what’s important to you. Usually 90% there is good enough.
It’s OK to Nit-Pick Syntax Issues Spelling Errors Poor Variable Names Missing corner-cases
It’s OK to Nit-Pick
Save the nit-picks for last, after any pressing architecture, design, or other large scale issues have been addressed.
Avoid Burn-OutStudies show reviewer should look at 200-400 lines of code at a time for maximum impact
https://smartbear.com/learn/code-review/best-practices-for-peer-code-review/
https://smartbear.com/learn/code-review/best-practices-for-peer-code-review/
Limit Reviews to 200-400 lines between 60 and 90 minutes, then take a break.
Avoid Burn-Out
Do Reviews in 24-48 hours
Define Done Let the submitter know if you approve, or if they need to make changes. Use consistent language
LGTM or Changes Requested Follow up when the submitter says they fixed something
Have Empathy Watch your Language Have Clear Feedback Give Compliments Don’t be a Perfectionist
How to be a Great Reviewer?
Avoid Burn Out Complete in 24-48 hours Define Done
How to be a Great Reviewer?
How to Be a Great Submitter
Submitter Reviewer
Don’t get rubber-stamped.
Don’t be too clever.
Readability Counts!
“
Good code is like a good joke. It needs no explanation.
Russ Olsen
Stages of Review 0 before submission 1 submitted 2 (optional) work in progress (30-50%) 3 almost done (90-100%) 4 review completed
step 0: before submission
Provide Context (The Why)
Link to the underlying ticket/issue Document why the change was needed For larger PRs, provide a changelog Point out any side-effects
YOU are the Primary Reviewer Review your code with the same level of detail you would give giving reviews. Anticipate problem areas
The Primary Reviewer Make sure your code works, and is thoroughly tested. Don’t rely on others to catch your mistakes.
Before Submitting, Try a CheckList
☑︎ small stuff Did you check for reusable code or utility methods? Did I remove debugger statements? Are there clear commit messages?
☑︎ big stuff Is my code secure? Will it scale? Read the Checklist Manifesto
step 1: submitted
You’re starting a conversation.
Don’t get too attached to your code before the review process Anticipate comments and feedback Acknowledge you will make mistakes
step 2: (optional)
work in progress
Submit Work in Progress PRs
Necessary for bigger features Open them your code is 30-50% done Don’t be afraid of showing incomplete, incremental work This is the right time to get feedback on bigger issues with the code
When Code is Work in Progress
Feedback to expect: Architectural Issues Problems with Overall Design Design Pattern Suggestions
step 3: almost done
When Code is Almost Done
Feedback to expect: Nit Picks Variable Names Documentation & Comments Small Optimizations
One Review per Issue
Solved an unrelated problem? make a new PR with a separate diff If you’re solving multiple problems, break them up into multiple PRs for ease of review. You can branch off your feature branch to keep the diff small
Prevent Reviewer Burnout
Remember, reviews lose value when they’re more than 500 lines of code. Keep reviews small and relevant. If submitting a big review is unavoidable, give the reviewer extra time.
https://www.microsoft.com/en-us/research/wp-content/uploads/2016/02/bosu2015useful.pdf
✔ Check Code with Automated Tools
✔ Lintercheck syntax or style guide violations
✔ Tests Write tests! Don’t know code health if tests are failing Tests Identify problems immediately
✔ Continuous Integration
automated build with every push
✔ Coverage% of code execute when running a test suite
step 4: review complete
Be Responsive Reply to every comment Common Responses:
Resolved Won’t Fix
If you won’t fix, make sure you’ve come to a mutual understanding with the reviewer
If there were comments, let your reviewer know when you’ve pushed changes and
are ready to be re-reviewed.
It’s still a Conversation
Don’t Bike-Shed bikeshed.com back & forth > 3 times? step away from the keyboard. talk instead! record the conversation in the Review
Fight for what you believe, but gracefully accept defeat.
Don’t take it personally.
It’s an opportunity for growth.
Provide the Why Review your own code Expect Conversation Submit in progress work
How to be a Great Submitter?
How to be a Great Submitter?Use automated tools Be Responsive Accept Defeat
Code Reviews
Build A Stronger
Team
Use that as an advantage when someone new joins!
On your first day…
Newbies
Not everyone has experience being reviewed. Remember what it felt like when you introduced the process.
Ease into it!
On-boarding
Start by reading recently completed reviews. The first submitted review is always the hardest First review should be small. Share the style guide
Everyone’s a Reviewer
Junior devs start by doing pair-reviews with a more experienced teammate. Use it as a mentorship opportunity.
“When your team succeeds, you succeed.”
“
Hiring senior engineers is hard. You can hire junior engineers, and grow
them into functional productive parts of your team.
Sasha Laundy
http://blog.sashalaundy.com/talks/asking-helping/
If you’re not doing code reviews, you’re missing a big
opportunity.
Remember… Allocate the time Develop, don’t force the culture Not one size fits all Or a one stop fix Use in addition to tests, QA, etc for maximum impact
What did we learn?
?
reviews decrease WTFs/mthe only valid measurement
of code quality
less WTFs ➡ happier devs!
bonus reading & resources at the end of the slides bit.ly/reviewcode
Bonus Material!
Resources & Additional ReadingMicrosoft Study on Effective Code Reviewhttps://www.microsoft.com/en-us/research/wp-content/uploads/2016/02/bosu2015useful.pdf
Code Reviews: Just do ithttps://blog.codinghorror.com/code-reviews-just-do-it/
Code Project - Code Review Guidelineshttp://www.codeproject.com/Articles/524235/Codeplusreviewplusguidelines%20and
Great Example Checklisthttp://insights.dice.com/2012/10/31/whats-on-my-code-review-checklist/
Best Practices for Code Reviewhttps://smartbear.com/learn/code-review/best-practices-for-peer-code-review/
Resources & Additional ReadingRebecca's Rules for Constructive Code Reviewhttps://storify.com/ReBeccaOrg/rebecca-s-rules-for-constructive-code-reviews
My Big Fat Scary Pull Request https://medium.com/@jdan/my-big-fat-scary-pull-request-2c8ac394540e#.yhs96vbxu
Hacker School Environment & Social Rules https://www.recurse.com/manual#sec-environment
Example Style GuidesPython https://www.python.org/dev/peps/pep-0008/
Scala https://github.com/paypal/scala-style-guide
Javascript https://github.com/airbnb/javascript
Google has many good, but strict style guideshttps://github.com/google/styleguide
Doesn't matter which one you use. Pick one and stick with it.
Credits
Special thanks to all the people who made and released these awesome resources for free:
Presentation template by SlidesCarnival