how to successfully grow a code review culture

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

✔ Coverage Coverage tools can integrate into GitHub.

coverage.py

coveralls.io

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!

@nnja bit.ly/reviewcode nina.writes.code@gmail.com

Thanks!

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