$30 off During Our Annual Pro Sale. View Details »

Code Review Skills For People

Code Review Skills For People

Presented at OSCON 2019: https://conferences.oreilly.com/oscon/oscon-or/public/schedule/detail/76240

As teams and projects grow, code review becomes increasingly important to support the maintainability of complex code bases. But code reviews aren’t as straightforward as they appear because the people involved in them aren’t always predictable.

Nina Zakharenko dives deep into guidelines for writing consistent code, linting and analysis tools for various languages, and common code review gotchas. You’ll learn about style guides and how they can help make your code more consistent and easier to maintain, as well as what tools are available to help automate the review process. You’ll have better code reviews with your teams at work by giving code reviews with empathy using reviews as tools for sharing knowledge instead of turning the process into a competition. You’ll also discover a better approach to code reviews in open source projects.

Nina Zakharenko

July 18, 2019
Tweet

More Decks by Nina Zakharenko

Other Decks in Technology

Transcript

  1. nina.to/oscon2019
    ---
    NINA ZAKHARENKO
    ---
    CODE REVIEW SKILLS
    ---
    @NNJA

    View Slide

  2. CODE REVIEW SKILLS FOR...
    ‣ Emotionally Intelligent Developers?
    ‣ ➡ Emotionally Intelligent
    Developers
    ‣ ➡ Developers
    ‣ ➡ For People
    @nnja

    View Slide

  3. whoami?
    @nnja
    nnja

    nina.to
    @nnja

    View Slide

  4. WHAT YOU'LL LEARN
    ‣ Proven benefits
    ‣ Standards
    ‣ Time saving tools & automation
    ‣ be the best reviewer
    ‣ Submit PRs for impact
    @nnja

    View Slide

  5. YOUR TAKE AWAYS
    ‣ Novice - Comprehensive overview
    of code review best practices
    ‣ Intermediate - Tooling &
    automation
    ‣ Advanced - People
    @nnja

    View Slide

  6. LIVETWEET
    #OSCON
    @NNJA

    View Slide

  7. NOT ONE SIZE FITS ALL
    ‣ team size
    ‣2 vs 10
    ‣ closed vs open source
    ‣ defect tolerance
    ‣low vs high
    @nnja

    View Slide

  8. WHY CODE REVIEW?
    @nnja

    View Slide

  9. CODE REVIEWS CAN
    BE FRUSTRATING
    @nnja

    View Slide

  10. "FRUSTRATIONS"
    ‣ + time demand
    ‣ + process
    ‣ team tension
    ‣“smart” devs think they don’t
    need it
    !
    @nnja

    View Slide

  11. CODE REVIEW BENEFITS
    @nnja

    View Slide

  12. SHARED OWNERSHIP & KNOWLEDGE
    ‣ We’re in this together
    ‣ No developer is the only
    expert
    ‣ ‑ lottery factor
    @nnja

    View Slide

  13. THE LOTTERY FACTOR
    New Yorker: Can Andy Byford Save the Subways?

    View Slide

  14. 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

    View Slide

  15. THE GOAL IS TO FIND
    BUGS BEFORE YOUR
    CUSTOMERS DO
    @nnja

    View Slide

  16. CODE REVIEW BENEFITS
    ‣ Find Bugs before customers &
    users
    ‣ Shared ownership
    ‣ Shared knowledge
    ‣ Reduce "Lottery Factor"
    @nnja

    View Slide

  17. HOW?
    @nnja

    View Slide

  18. ENCOURAGE CONSISTENCY
    ‣ Work for a company?
    ‣Your code isn’t yours
    ‣ code should fit project's
    style
    ‣not your own
    @nnja

    View Slide

  19. STYLE GUIDE
    ‣ Distinguishes personal taste
    from opinion
    ‣ Agreed on beforehand
    ‣ Example: Google's pyguide.md
    @nnja

    View Slide

  20. REVIEW IS UNIVERSAL
    ‣ Doesn’t matter how junior or
    senior you are
    ‣ Inequality breeds
    dissatisfaction
    @nnja

    View Slide

  21. CONSISTENT CODE IS
    EASIER TO MAINTAIN
    BY A TEAM
    @nnja

    View Slide

  22. CODE REVIEW SHOULD BE DONE
    BY YOUR PEERS
    @nnja

    View Slide

  23. DON’T POINT FINGERS!
    @nnja

    View Slide

  24. WHEN CODE REVIEWS ARE POSITIVE,
    DEVELOPERS
    DON’T JUST EXPECT REVIEWS,
    THEY LOOK FORWARD TO THEM
    @nnja

    View Slide

  25. REVIEW FUNDAMENTALS
    ‣ Performed by peers
    ‣ Style guides for consistency
    ‣ No blame culture
    @nnja

    View Slide

  26. HOW TO REVIEW CODE?
    BE A GREAT REVIEWER
    AND
    A GREAT SUBMITTER
    @nnja

    View Slide

  27. BE A GREAT
    REVIEWER
    @nnja

    View Slide

  28. SELF-CARE
    ☕"#$
    @nnja

    View Slide

  29. 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

    View Slide

  30. LIMIT REVIEWS TO 400 LINES IN 60 MINS
    TO MAXIMIZE EFFECTIVENESS3
    3
    https://smartbear.com/learn/code-review/best-practices-for-peer-code-review/

    View Slide

  31. @nnja

    View Slide

  32. BE OBJECTIVE:
    "THIS METHOD IS MISSING
    DOCUMENTATION"
    INSTEAD OF
    “YOU FORGOT TO WRITE
    DOCUMENTATION"
    @nnja

    View Slide

  33. ASK QUESTIONS, DON’T GIVE ANSWERS
    ‣ “Would it make more sense
    if... ?”
    ‣ “Did you think about... ?”
    ‣ "What about trade-offs... ?"
    @nnja

    View Slide

  34. OFFER SUGGESTIONS
    ‣ “It might be easier to...”
    ‣ “We tend to do it this way...”
    @nnja

    View Slide

  35. AVOID THESE TERMS
    ‣ Simply
    ‣ Easily
    ‣ Just
    ‣ Obviously
    ‣ Well, actually...
    Recurse Center Social Rules

    View Slide

  36. ... NOW, SIMPLY
    @nnja

    View Slide

  37. HAVE CLEAR FEEDBACK
    ‣ Strongly support your opinions
    ‣ Share How & Why
    ‣ Link to supporting docs,
    stackoverflow, etc
    ‣ Enforce the style guide
    @nnja

    View Slide

  38. COMPLIMENT GOOD WORK
    & GREAT IDEAS
    @nnja

    View Slide

  39. @nnja

    View Slide

  40. 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

    View Slide

  41. IT’S OK TO NIT-PICK
    ‣ Syntax issues
    ‣ Spelling errors
    ‣ Poor variable names
    ‣ Missing corner-cases
    @nnja

    View Slide

  42. SAVE NIT-PICKS FOR LAST
    @nnja

    View Slide

  43. TRY TO DO REVIEWS IN 24-48 HOURS
    @nnja

    View Slide

  44. BE A GREAT REVIEWER
    ‣ Empathy
    ‣ Watch your language
    ‣ Clear feedback
    ‣ Give compliments
    @nnja

    View Slide

  45. BE A GREAT REVIEWER
    ‣ Don’t be a perfectionist
    ‣ Avoid burn out
    ‣ Finish in 24-48 hours
    ‣(if possible)
    @nnja

    View Slide

  46. SUBMIT GREAT
    PRS
    @nnja

    View Slide

  47. DON’T GET RUBBER-STAMPED.
    @nnja

    View Slide

  48. View Slide

  49. DON’T BE CLEVER.
    READABILITY COUNTS!
    @nnja

    View Slide

  50. GOOD CODE IS LIKE A GOOD JOKE.
    IT NEEDS NO EXPLANATION.
    - RUSS OLSEN
    @nnja

    View Slide

  51. REVIEW LIFECYCLE
    ‣ 0: Before submission
    ‣ 1: Submitted
    ‣ 2: Work in progress PR (optional at
    30-50%)
    ‣ 3: Almost done (90-100%)
    ‣ 4: Completed
    @nnja

    View Slide

  52. STEP 0:
    BEFORE SUBMISSION
    @nnja

    View Slide

  53. DID YOU SOLVE ONE ISSUE?
    ‣ Solving multiple issues?
    ‣Break them up into multiple
    PRs
    ‣ Also solved an unrelated issue?
    ‣Make a new PR
    @nnja

    View Slide

  54. 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

    View Slide

  55. 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

    View Slide

  56. YOU ARE THE
    PRIMARY REVIEWER
    ‣ Review your own code as if it
    belonged to someone else
    ‣ Anticipate problem areas
    @nnja

    View Slide

  57. AS PRIMARY REVIEWER
    ‣ Make sure your code works, and
    is thoroughly tested
    ‣ Don’t rely on others to catch
    your mistakes
    @nnja

    View Slide

  58. ✔ CHECK CODE
    WITH AUTOMATED TOOLS
    @nnja

    View Slide

  59. DYNAMIC CODE? USE A LINTER✔
    @nnja

    View Slide

  60. 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

    View Slide

  61. 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

    View Slide

  62. pre-commit.com
    @nnja

    View Slide

  63. View Slide

  64. ✔ TESTS
    ‣ Write them!
    ‣ Don’t know code health if tests are
    failing
    ‣ Tests identify problems immediately
    @nnja

    View Slide

  65. ✔ CONTINUOUS INTEGRATION
    CPYTHON USES
    Azure DevOps Pipelines
    @nnja

    View Slide

  66. ✔ COVERAGE
    % OF CODE EXECUTED WHEN RUNNING A TEST SUITE
    @nnja

    View Slide

  67. BEFORE SUBMITTING, TRY A CHECKLIST
    @nnja

    View Slide

  68. ☑ SMALL STUFF
    ‣ ☑ check for reusable code
    ‣ ☑ use utility methods
    ‣ ☑ remove debugger statements?
    ‣ ☑ write clear commit messages?
    ‣ ☑ small PR?
    @nnja

    View Slide

  69. ☑ 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

    View Slide

  70. STEP 1:
    SUBMITTED
    @nnja

    View Slide

  71. YOU’RE STARTING A CONVERSATION
    ‣ don't get attached
    ‣ anticipate feedback
    ‣ acknowledge you'll make
    mistakes
    @nnja

    View Slide

  72. (OPTIONAL)
    STEP 2:
    WORK IN
    PROGRESS
    @nnja

    View Slide

  73. 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

    View Slide

  74. WHEN PR IS WORK IN PROGRESS
    ‣ Feedback to expect:
    ‣Architectural issues
    ‣Problems with overall design
    ‣Design pattern suggestions
    @nnja

    View Slide

  75. STEP 3:
    ALMOST DONE
    @nnja

    View Slide

  76. WHEN CODE IS ALMOST DONE
    ‣ Feedback to expect:
    ‣Nit picks
    ‣Variable names
    ‣Documentation & comments
    ‣Small optimizations
    @nnja

    View Slide

  77. 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

    View Slide

  78. FIGHT FOR WHAT YOU BELIEVE, BUT
    GRACEFULLY ACCEPT DEFEAT
    @nnja

    View Slide

  79. 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

    View Slide

  80. VS CODE LIVE SHARE
    Download Extension

    View Slide

  81. DON’T TAKE FEEDBACK PERSONALLY.
    IT’S AN OPPORTUNITY FOR GROWTH
    @nnja

    View Slide

  82. HOW TO SUBMIT GREAT PRS?
    ‣ Provide the why (context!)
    ‣ Review your own code
    ‣ Expect conversation
    ‣ Submit in progress work
    @nnja

    View Slide

  83. HOW TO SUBMIT GREAT PRS?
    ‣ Use automated tools
    ‣ Be responsive
    ‣ Accept defeat
    @nnja

    View Slide

  84. I'VE BECOME A BETTER DEVELOPER
    BY PARTICIPATING IN CODE REVIEWS
    @nnja

    View Slide

  85. CODE
    REVIEWS
    BUILD A
    STRONGER
    TEAM
    @nnja

    View Slide

  86. HIRING SENIOR ENGINEERS IS HARD.
    YOU CAN HIRE JUNIOR ENGINEERS,
    AND GROW THEM
    INTO FUNCTIONAL PRODUCTIVE PARTS OF
    YOUR TEAM.
    -- Sasha Laundy
    @nnja

    View Slide

  87. CONSIDER...
    ‣ Develop, don’t force the culture
    ‣ Not one size fits all
    ‣ Use in addition to tests, QA, etc
    for maximum impact
    @nnja

    View Slide

  88. @nnja

    View Slide

  89. PLEASE,
    BE KIND
    TO EACH OTHER
    @nnja

    View Slide

  90. THANKS!
    @NNJA
    SLIDES: nina.to/oscon2019
    PYTHON @ MICROSOFT: aka.ms/py-oscon
    (ADDITIONAL READING ON NEXT SLIDE)

    View Slide

  91. 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

    View Slide