Upgrade to Pro — share decks privately, control downloads, hide ads and more …

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

More Decks by Nina Zakharenko

Other Decks in Technology


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

    Emotionally Intelligent Developers ‣ ➡ Developers ‣ ➡ For People @nnja
  2. WHAT YOU'LL LEARN ‣ Proven benefits ‣ Standards ‣ Time

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

    review best practices ‣ Intermediate - Tooling & automation ‣ Advanced - People @nnja
  4. NOT ONE SIZE FITS ALL ‣ team size ‣2 vs

    10 ‣ closed vs open source ‣ defect tolerance ‣low vs high @nnja
  5. "FRUSTRATIONS" ‣ + time demand ‣ + process ‣ team

    tension ‣“smart” devs think they don’t need it ! @nnja
  6. SHARED OWNERSHIP & KNOWLEDGE ‣ We’re in this together ‣

    No developer is the only expert ‣ ‑ lottery factor @nnja
  7. 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
  8. CODE REVIEW BENEFITS ‣ Find Bugs before customers & users

    ‣ Shared ownership ‣ Shared knowledge ‣ Reduce "Lottery Factor" @nnja
  9. ENCOURAGE CONSISTENCY ‣ Work for a company? ‣Your code isn’t

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

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

    you are ‣ Inequality breeds dissatisfaction @nnja
  12. 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

    EFFECTIVENESS3 3 https://smartbear.com/learn/code-review/best-practices-for-peer-code-review/
  14. ASK QUESTIONS, DON’T GIVE ANSWERS ‣ “Would it make more

    sense if... ?” ‣ “Did you think about... ?” ‣ "What about trade-offs... ?" @nnja
  15. AVOID THESE TERMS ‣ Simply ‣ Easily ‣ Just ‣

    Obviously ‣ Well, actually... Recurse Center Social Rules
  16. HAVE CLEAR FEEDBACK ‣ Strongly support your opinions ‣ Share

    How & Why ‣ Link to supporting docs, stackoverflow, etc ‣ Enforce the style guide @nnja
  17. 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
  18. IT’S OK TO NIT-PICK ‣ Syntax issues ‣ Spelling errors

    ‣ Poor variable names ‣ Missing corner-cases @nnja
  19. BE A GREAT REVIEWER ‣ Empathy ‣ Watch your language

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

    Avoid burn out ‣ Finish in 24-48 hours ‣(if possible) @nnja

  22. REVIEW LIFECYCLE ‣ 0: Before submission ‣ 1: Submitted ‣

    2: Work in progress PR (optional at 30-50%) ‣ 3: Almost done (90-100%) ‣ 4: Completed @nnja
  23. DID YOU SOLVE ONE ISSUE? ‣ Solving multiple issues? ‣Break

    them up into multiple PRs ‣ Also solved an unrelated issue? ‣Make a new PR @nnja
  24. 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
  25. 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
  26. YOU ARE THE PRIMARY REVIEWER ‣ Review your own code

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

    is thoroughly tested ‣ Don’t rely on others to catch your mistakes @nnja
  28. 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
  29. 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
  30. ✔ TESTS ‣ Write them! ‣ Don’t know code health

    if tests are failing ‣ Tests identify problems immediately @nnja
  31. ☑ SMALL STUFF ‣ ☑ check for reusable code ‣

    ☑ use utility methods ‣ ☑ remove debugger statements? ‣ ☑ write clear commit messages? ‣ ☑ small PR? @nnja
  32. ☑ 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
  33. YOU’RE STARTING A CONVERSATION ‣ don't get attached ‣ anticipate

    feedback ‣ acknowledge you'll make mistakes @nnja
  34. 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
  35. WHEN PR IS WORK IN PROGRESS ‣ Feedback to expect:

    ‣Architectural issues ‣Problems with overall design ‣Design pattern suggestions @nnja
  36. WHEN CODE IS ALMOST DONE ‣ Feedback to expect: ‣Nit

    picks ‣Variable names ‣Documentation & comments ‣Small optimizations @nnja
  37. 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
  38. 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
  39. HOW TO SUBMIT GREAT PRS? ‣ Provide the why (context!)

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

    Be responsive ‣ Accept defeat @nnja

  42. CONSIDER... ‣ Develop, don’t force the culture ‣ Not one

    size fits all ‣ Use in addition to tests, QA, etc for maximum impact @nnja
  43. 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