Slide 1
Slide 1 text
nina.to/oscon2019
---
NINA ZAKHARENKO
---
CODE REVIEW SKILLS
---
@NNJA
Slide 2
Slide 2 text
CODE REVIEW SKILLS FOR...
‣ Emotionally Intelligent Developers?
‣ ➡ Emotionally Intelligent
Developers
‣ ➡ Developers
‣ ➡ For People
@nnja
Slide 3
Slide 3 text
whoami?
@nnja
nnja
✏
nina.to
@nnja
Slide 4
Slide 4 text
WHAT YOU'LL LEARN
‣ Proven benefits
‣ Standards
‣ Time saving tools & automation
‣ be the best reviewer
‣ Submit PRs for impact
@nnja
Slide 5
Slide 5 text
YOUR TAKE AWAYS
‣ Novice - Comprehensive overview
of code review best practices
‣ Intermediate - Tooling &
automation
‣ Advanced - People
@nnja
Slide 6
Slide 6 text
LIVETWEET
#OSCON
@NNJA
Slide 7
Slide 7 text
NOT ONE SIZE FITS ALL
‣ team size
‣2 vs 10
‣ closed vs open source
‣ defect tolerance
‣low vs high
@nnja
Slide 8
Slide 8 text
WHY CODE REVIEW?
@nnja
Slide 9
Slide 9 text
CODE REVIEWS CAN
BE FRUSTRATING
@nnja
Slide 10
Slide 10 text
"FRUSTRATIONS"
‣ + time demand
‣ + process
‣ team tension
‣“smart” devs think they don’t
need it
!
@nnja
Slide 11
Slide 11 text
CODE REVIEW BENEFITS
@nnja
Slide 12
Slide 12 text
SHARED OWNERSHIP & KNOWLEDGE
‣ We’re in this together
‣ No developer is the only
expert
‣ ‑ lottery factor
@nnja
Slide 13
Slide 13 text
THE LOTTERY FACTOR
New Yorker: Can Andy Byford Save the Subways?
Slide 14
Slide 14 text
FIND BUGS & DESIGN FLAWS
‣ find bugs before the code is done
‣ Case Studies on Review1:
‣bug rate ‑ by 80%
‣productivity ‐ by 15%
1 Code Reviews: Just do it
@nnja
Slide 15
Slide 15 text
THE GOAL IS TO FIND
BUGS BEFORE YOUR
CUSTOMERS DO
@nnja
Slide 16
Slide 16 text
CODE REVIEW BENEFITS
‣ Find Bugs before customers &
users
‣ Shared ownership
‣ Shared knowledge
‣ Reduce "Lottery Factor"
@nnja
Slide 17
Slide 17 text
HOW?
@nnja
Slide 18
Slide 18 text
ENCOURAGE CONSISTENCY
‣ Work for a company?
‣Your code isn’t yours
‣ code should fit project's
style
‣not your own
@nnja
Slide 19
Slide 19 text
STYLE GUIDE
‣ Distinguishes personal taste
from opinion
‣ Agreed on beforehand
‣ Example: Google's pyguide.md
@nnja
Slide 20
Slide 20 text
REVIEW IS UNIVERSAL
‣ Doesn’t matter how junior or
senior you are
‣ Inequality breeds
dissatisfaction
@nnja
Slide 21
Slide 21 text
CONSISTENT CODE IS
EASIER TO MAINTAIN
BY A TEAM
@nnja
Slide 22
Slide 22 text
CODE REVIEW SHOULD BE DONE
BY YOUR PEERS
@nnja
Slide 23
Slide 23 text
DON’T POINT FINGERS!
@nnja
Slide 24
Slide 24 text
WHEN CODE REVIEWS ARE POSITIVE,
DEVELOPERS
DON’T JUST EXPECT REVIEWS,
THEY LOOK FORWARD TO THEM
@nnja
Slide 25
Slide 25 text
REVIEW FUNDAMENTALS
‣ Performed by peers
‣ Style guides for consistency
‣ No blame culture
@nnja
Slide 26
Slide 26 text
HOW TO REVIEW CODE?
BE A GREAT REVIEWER
AND
A GREAT SUBMITTER
@nnja
Slide 27
Slide 27 text
BE A GREAT
REVIEWER
@nnja
Slide 28
Slide 28 text
SELF-CARE
☕"#$
@nnja
Slide 29
Slide 29 text
DON'T BURN OUT
Studies show reviewer should look at
200-400 lines of code at a time for maximum impact.2
2 https://www.microsoft.com/en-us/research/wp-content/uploads/
2016/02/bosu2015useful.pdf
@nnja
Slide 30
Slide 30 text
LIMIT REVIEWS TO 400 LINES IN 60 MINS
TO MAXIMIZE EFFECTIVENESS3
3
https://smartbear.com/learn/code-review/best-practices-for-peer-code-review/
Slide 31
Slide 31 text
@nnja
Slide 32
Slide 32 text
BE OBJECTIVE:
"THIS METHOD IS MISSING
DOCUMENTATION"
INSTEAD OF
“YOU FORGOT TO WRITE
DOCUMENTATION"
@nnja
Slide 33
Slide 33 text
ASK QUESTIONS, DON’T GIVE ANSWERS
‣ “Would it make more sense
if... ?”
‣ “Did you think about... ?”
‣ "What about trade-offs... ?"
@nnja
Slide 34
Slide 34 text
OFFER SUGGESTIONS
‣ “It might be easier to...”
‣ “We tend to do it this way...”
@nnja
Slide 35
Slide 35 text
AVOID THESE TERMS
‣ Simply
‣ Easily
‣ Just
‣ Obviously
‣ Well, actually...
Recurse Center Social Rules
Slide 36
Slide 36 text
... NOW, SIMPLY
@nnja
Slide 37
Slide 37 text
HAVE CLEAR FEEDBACK
‣ Strongly support your opinions
‣ Share How & Why
‣ Link to supporting docs,
stackoverflow, etc
‣ Enforce the style guide
@nnja
Slide 38
Slide 38 text
COMPLIMENT GOOD WORK
& GREAT IDEAS
@nnja
Slide 39
Slide 39 text
@nnja
Slide 40
Slide 40 text
DON’T BE A PERFECTIONIST
‣ Don’t let perfect get in the way
of perfectly acceptable.
‣ Prioritize what’s important to
you.
‣ Usually 90% there is good enough.
@nnja
Slide 41
Slide 41 text
IT’S OK TO NIT-PICK
‣ Syntax issues
‣ Spelling errors
‣ Poor variable names
‣ Missing corner-cases
@nnja
Slide 42
Slide 42 text
SAVE NIT-PICKS FOR LAST
@nnja
Slide 43
Slide 43 text
TRY TO DO REVIEWS IN 24-48 HOURS
@nnja
Slide 44
Slide 44 text
BE A GREAT REVIEWER
‣ Empathy
‣ Watch your language
‣ Clear feedback
‣ Give compliments
@nnja
Slide 45
Slide 45 text
BE A GREAT REVIEWER
‣ Don’t be a perfectionist
‣ Avoid burn out
‣ Finish in 24-48 hours
‣(if possible)
@nnja
Slide 46
Slide 46 text
SUBMIT GREAT
PRS
@nnja
Slide 47
Slide 47 text
DON’T GET RUBBER-STAMPED.
@nnja
Slide 48
Slide 48 text
No content
Slide 49
Slide 49 text
DON’T BE CLEVER.
READABILITY COUNTS!
@nnja
Slide 50
Slide 50 text
GOOD CODE IS LIKE A GOOD JOKE.
IT NEEDS NO EXPLANATION.
- RUSS OLSEN
@nnja
Slide 51
Slide 51 text
REVIEW LIFECYCLE
‣ 0: Before submission
‣ 1: Submitted
‣ 2: Work in progress PR (optional at
30-50%)
‣ 3: Almost done (90-100%)
‣ 4: Completed
@nnja
Slide 52
Slide 52 text
STEP 0:
BEFORE SUBMISSION
@nnja
Slide 53
Slide 53 text
DID YOU SOLVE ONE ISSUE?
‣ Solving multiple issues?
‣Break them up into multiple
PRs
‣ Also solved an unrelated issue?
‣Make a new PR
@nnja
Slide 54
Slide 54 text
SMALL PRS PREVENT REVIEWER BURNOUT
‣ Reviews lose value when they’re
more than 500 lines of code2
‣ Keep them small & relevant
‣ If a big PR is unavoidable, give
the reviewer extra time
2 https://www.microsoft.com/en-us/research/wp-content/uploads/
2016/02/bosu2015useful.pdf
@nnja
Slide 55
Slide 55 text
CONTEXT (THE WHY)
‣ What was the motivation for
submitting this code?
‣Link to issue
‣Document why the change was needed
‣large PR? provide a changelog
‣ Point out side-effects
@nnja
Slide 56
Slide 56 text
YOU ARE THE
PRIMARY REVIEWER
‣ Review your own code as if it
belonged to someone else
‣ Anticipate problem areas
@nnja
Slide 57
Slide 57 text
AS PRIMARY REVIEWER
‣ Make sure your code works, and
is thoroughly tested
‣ Don’t rely on others to catch
your mistakes
@nnja
Slide 58
Slide 58 text
✔ CHECK CODE
WITH AUTOMATED TOOLS
@nnja
Slide 59
Slide 59 text
DYNAMIC CODE? USE A LINTER✔
@nnja
Slide 60
Slide 60 text
LEARN YOUR TOOLS
foo = [
1,
2,
3,
4]
bar = 1, 2, # bar is a Tuple of (1, 2)
bar = 2, # bar is still a Tuple
bar = 2 # bar is now a number
Caught with linter rule: trailing-comma-tuple
@nnja
Slide 61
Slide 61 text
GIT PRE-COMMIT HOOKS
‣ run linter
‣ check syntax
‣ check for TODOs, debugger statements,
unused imports
‣ enforce styling
‣ reject commit if conditions aren't met
@nnja
Slide 62
Slide 62 text
pre-commit.com
@nnja
Slide 63
Slide 63 text
No content
Slide 64
Slide 64 text
✔ TESTS
‣ Write them!
‣ Don’t know code health if tests are
failing
‣ Tests identify problems immediately
@nnja
Slide 65
Slide 65 text
✔ CONTINUOUS INTEGRATION
CPYTHON USES
Azure DevOps Pipelines
@nnja
Slide 66
Slide 66 text
✔ COVERAGE
% OF CODE EXECUTED WHEN RUNNING A TEST SUITE
@nnja
Slide 67
Slide 67 text
BEFORE SUBMITTING, TRY A CHECKLIST
@nnja
Slide 68
Slide 68 text
☑ SMALL STUFF
‣ ☑ check for reusable code
‣ ☑ use utility methods
‣ ☑ remove debugger statements?
‣ ☑ write clear commit messages?
‣ ☑ small PR?
@nnja
Slide 69
Slide 69 text
☑ BIG STUFF
‣ ☑ Is my code secure?
‣ ☑ Will it scale?
‣ ☑ Is it easy to maintain?
‣ ☑ Is it resilient against outages?
Tip: Read The Checklist Manifesto
@nnja
Slide 70
Slide 70 text
STEP 1:
SUBMITTED
@nnja
Slide 71
Slide 71 text
YOU’RE STARTING A CONVERSATION
‣ don't get attached
‣ anticipate feedback
‣ acknowledge you'll make
mistakes
@nnja
Slide 72
Slide 72 text
(OPTIONAL)
STEP 2:
WORK IN
PROGRESS
@nnja
Slide 73
Slide 73 text
SUBMIT WORK IN PROGRESS PRS
‣ Good idea for bigger features
‣ Open when code is 30-50% done
‣ Don’t be afraid of showing
incomplete, incremental work
@nnja
Slide 74
Slide 74 text
WHEN PR IS WORK IN PROGRESS
‣ Feedback to expect:
‣Architectural issues
‣Problems with overall design
‣Design pattern suggestions
@nnja
Slide 75
Slide 75 text
STEP 3:
ALMOST DONE
@nnja
Slide 76
Slide 76 text
WHEN CODE IS ALMOST DONE
‣ Feedback to expect:
‣Nit picks
‣Variable names
‣Documentation & comments
‣Small optimizations
@nnja
Slide 77
Slide 77 text
BE RESPONSIVE
(IT’S STILL A CONVERSATION)
‣ Respond to comments!
‣(even if you don't agree with them)
‣ Don't think it's necessary?
‣come to a mutual understanding about why
@nnja
Slide 78
Slide 78 text
FIGHT FOR WHAT YOU BELIEVE, BUT
GRACEFULLY ACCEPT DEFEAT
@nnja
Slide 79
Slide 79 text
DON’T BIKE-SHED
‣ Back and forth > 3 times?
‣Step away from the keyboard
‣ Talk instead!
‣Record the results of the
conversation in the PR
@nnja
Slide 80
Slide 80 text
VS CODE LIVE SHARE
Download Extension
Slide 81
Slide 81 text
DON’T TAKE FEEDBACK PERSONALLY.
IT’S AN OPPORTUNITY FOR GROWTH
@nnja
Slide 82
Slide 82 text
HOW TO SUBMIT GREAT PRS?
‣ Provide the why (context!)
‣ Review your own code
‣ Expect conversation
‣ Submit in progress work
@nnja
Slide 83
Slide 83 text
HOW TO SUBMIT GREAT PRS?
‣ Use automated tools
‣ Be responsive
‣ Accept defeat
@nnja
Slide 84
Slide 84 text
I'VE BECOME A BETTER DEVELOPER
BY PARTICIPATING IN CODE REVIEWS
@nnja
Slide 85
Slide 85 text
CODE
REVIEWS
BUILD A
STRONGER
TEAM
@nnja
Slide 86
Slide 86 text
HIRING SENIOR ENGINEERS IS HARD.
YOU CAN HIRE JUNIOR ENGINEERS,
AND GROW THEM
INTO FUNCTIONAL PRODUCTIVE PARTS OF
YOUR TEAM.
-- Sasha Laundy
@nnja
Slide 87
Slide 87 text
CONSIDER...
‣ Develop, don’t force the culture
‣ Not one size fits all
‣ Use in addition to tests, QA, etc
for maximum impact
@nnja
Slide 88
Slide 88 text
@nnja
Slide 89
Slide 89 text
PLEASE,
BE KIND
TO EACH OTHER
@nnja
Slide 90
Slide 90 text
THANKS!
@NNJA
SLIDES: nina.to/oscon2019
PYTHON @ MICROSOFT: aka.ms/py-oscon
(ADDITIONAL READING ON NEXT SLIDE)
Slide 91
Slide 91 text
RESOURCES & ADDITIONAL READING
‣ Top 3 Django Gotchas to Catch during Code Review
‣ Microsoft Study on Effective Code Review
‣ Code Reviews: Just do it
‣ Code Project - Code Review Guidelines
‣ Great Example Checklist
‣ Best Practices for Code Review
‣ Rebecca's Rules for Constructive Code Review
‣ My Big Fat Scary Pull Request
‣ The Gentle Art of Patch Review - Sage Sharp
‣ Watch: Raymond Hettinger - Beyond PEP8
@nnja