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.

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