Pro Yearly is on sale from $80 to $50! »

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.

E84b0476f3dc431aac7c8f5c71ed032b?s=128

Nina Zakharenko

July 18, 2019
Tweet

Transcript

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

  2. CODE REVIEW SKILLS FOR... ‣ Emotionally Intelligent Developers? ‣ ➡

    Emotionally Intelligent Developers ‣ ➡ Developers ‣ ➡ For People @nnja
  3. whoami? @nnja nnja ✏ nina.to @nnja

  4. WHAT YOU'LL LEARN ‣ Proven benefits ‣ Standards ‣ Time

    saving tools & automation ‣ be the best reviewer ‣ Submit PRs for impact @nnja
  5. YOUR TAKE AWAYS ‣ Novice - Comprehensive overview of code

    review best practices ‣ Intermediate - Tooling & automation ‣ Advanced - People @nnja
  6. LIVETWEET #OSCON @NNJA

  7. NOT ONE SIZE FITS ALL ‣ team size ‣2 vs

    10 ‣ closed vs open source ‣ defect tolerance ‣low vs high @nnja
  8. WHY CODE REVIEW? @nnja

  9. CODE REVIEWS CAN BE FRUSTRATING @nnja

  10. "FRUSTRATIONS" ‣ + time demand ‣ + process ‣ team

    tension ‣“smart” devs think they don’t need it ! @nnja
  11. CODE REVIEW BENEFITS @nnja

  12. SHARED OWNERSHIP & KNOWLEDGE ‣ We’re in this together ‣

    No developer is the only expert ‣ ‑ lottery factor @nnja
  13. THE LOTTERY FACTOR New Yorker: Can Andy Byford Save the

    Subways?
  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
  15. THE GOAL IS TO FIND BUGS BEFORE YOUR CUSTOMERS DO

    @nnja
  16. CODE REVIEW BENEFITS ‣ Find Bugs before customers & users

    ‣ Shared ownership ‣ Shared knowledge ‣ Reduce "Lottery Factor" @nnja
  17. HOW? @nnja

  18. ENCOURAGE CONSISTENCY ‣ Work for a company? ‣Your code isn’t

    yours ‣ code should fit project's style ‣not your own @nnja
  19. STYLE GUIDE ‣ Distinguishes personal taste from opinion ‣ Agreed

    on beforehand ‣ Example: Google's pyguide.md @nnja
  20. REVIEW IS UNIVERSAL ‣ Doesn’t matter how junior or senior

    you are ‣ Inequality breeds dissatisfaction @nnja
  21. CONSISTENT CODE IS EASIER TO MAINTAIN BY A TEAM @nnja

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

  23. DON’T POINT FINGERS! @nnja

  24. WHEN CODE REVIEWS ARE POSITIVE, DEVELOPERS DON’T JUST EXPECT REVIEWS,

    THEY LOOK FORWARD TO THEM @nnja
  25. REVIEW FUNDAMENTALS ‣ Performed by peers ‣ Style guides for

    consistency ‣ No blame culture @nnja
  26. HOW TO REVIEW CODE? BE A GREAT REVIEWER AND A

    GREAT SUBMITTER @nnja
  27. BE A GREAT REVIEWER @nnja

  28. SELF-CARE ☕"#$ @nnja

  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
  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/
  31. @nnja

  32. BE OBJECTIVE: "THIS METHOD IS MISSING DOCUMENTATION" INSTEAD OF “YOU

    FORGOT TO WRITE DOCUMENTATION" @nnja
  33. ASK QUESTIONS, DON’T GIVE ANSWERS ‣ “Would it make more

    sense if... ?” ‣ “Did you think about... ?” ‣ "What about trade-offs... ?" @nnja
  34. OFFER SUGGESTIONS ‣ “It might be easier to...” ‣ “We

    tend to do it this way...” @nnja
  35. AVOID THESE TERMS ‣ Simply ‣ Easily ‣ Just ‣

    Obviously ‣ Well, actually... Recurse Center Social Rules
  36. ... NOW, SIMPLY @nnja

  37. HAVE CLEAR FEEDBACK ‣ Strongly support your opinions ‣ Share

    How & Why ‣ Link to supporting docs, stackoverflow, etc ‣ Enforce the style guide @nnja
  38. COMPLIMENT GOOD WORK & GREAT IDEAS @nnja

  39. @nnja

  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
  41. IT’S OK TO NIT-PICK ‣ Syntax issues ‣ Spelling errors

    ‣ Poor variable names ‣ Missing corner-cases @nnja
  42. SAVE NIT-PICKS FOR LAST @nnja

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

  44. BE A GREAT REVIEWER ‣ Empathy ‣ Watch your language

    ‣ Clear feedback ‣ Give compliments @nnja
  45. BE A GREAT REVIEWER ‣ Don’t be a perfectionist ‣

    Avoid burn out ‣ Finish in 24-48 hours ‣(if possible) @nnja
  46. SUBMIT GREAT PRS @nnja

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

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

  50. GOOD CODE IS LIKE A GOOD JOKE. IT NEEDS NO

    EXPLANATION. - RUSS OLSEN @nnja
  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
  52. STEP 0: BEFORE SUBMISSION @nnja

  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
  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
  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
  56. YOU ARE THE PRIMARY REVIEWER ‣ Review your own code

    as if it belonged to someone else ‣ Anticipate problem areas @nnja
  57. AS PRIMARY REVIEWER ‣ Make sure your code works, and

    is thoroughly tested ‣ Don’t rely on others to catch your mistakes @nnja
  58. ✔ CHECK CODE WITH AUTOMATED TOOLS @nnja

  59. DYNAMIC CODE? USE A LINTER✔ @nnja

  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
  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
  62. pre-commit.com @nnja

  63. None
  64. ✔ TESTS ‣ Write them! ‣ Don’t know code health

    if tests are failing ‣ Tests identify problems immediately @nnja
  65. ✔ CONTINUOUS INTEGRATION CPYTHON USES Azure DevOps Pipelines @nnja

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

    SUITE @nnja
  67. BEFORE SUBMITTING, TRY A CHECKLIST @nnja

  68. ☑ SMALL STUFF ‣ ☑ check for reusable code ‣

    ☑ use utility methods ‣ ☑ remove debugger statements? ‣ ☑ write clear commit messages? ‣ ☑ small PR? @nnja
  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
  70. STEP 1: SUBMITTED @nnja

  71. YOU’RE STARTING A CONVERSATION ‣ don't get attached ‣ anticipate

    feedback ‣ acknowledge you'll make mistakes @nnja
  72. (OPTIONAL) STEP 2: WORK IN PROGRESS @nnja

  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
  74. WHEN PR IS WORK IN PROGRESS ‣ Feedback to expect:

    ‣Architectural issues ‣Problems with overall design ‣Design pattern suggestions @nnja
  75. STEP 3: ALMOST DONE @nnja

  76. WHEN CODE IS ALMOST DONE ‣ Feedback to expect: ‣Nit

    picks ‣Variable names ‣Documentation & comments ‣Small optimizations @nnja
  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
  78. FIGHT FOR WHAT YOU BELIEVE, BUT GRACEFULLY ACCEPT DEFEAT @nnja

  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
  80. VS CODE LIVE SHARE Download Extension

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

  82. HOW TO SUBMIT GREAT PRS? ‣ Provide the why (context!)

    ‣ Review your own code ‣ Expect conversation ‣ Submit in progress work @nnja
  83. HOW TO SUBMIT GREAT PRS? ‣ Use automated tools ‣

    Be responsive ‣ Accept defeat @nnja
  84. I'VE BECOME A BETTER DEVELOPER BY PARTICIPATING IN CODE REVIEWS

    @nnja
  85. CODE REVIEWS BUILD A STRONGER TEAM @nnja

  86. HIRING SENIOR ENGINEERS IS HARD. YOU CAN HIRE JUNIOR ENGINEERS,

    AND GROW THEM INTO FUNCTIONAL PRODUCTIVE PARTS OF YOUR TEAM. -- Sasha Laundy @nnja
  87. CONSIDER... ‣ Develop, don’t force the culture ‣ Not one

    size fits all ‣ Use in addition to tests, QA, etc for maximum impact @nnja
  88. @nnja

  89. PLEASE, BE KIND TO EACH OTHER @nnja

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

    ON NEXT 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