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