Effective Code Review

Peer code review is an essential part of the software engineering process in any team or open source setting, but since most of us received little training in how to do it, the quality of code reviews can vary wildly.

Based on my experience performing thousands of code reviews in large commercial and open source projects, I've developed strategies for making each review as effective as possible. In this talk, I'll present my process, along with the first principles that inspired it, to make your reviews much more valuable for everyone—the author, your teammates, and you (the reviewer) too!

Slides and presenter notes available at github.com/jspahrsummers/effective-code-review.

Transcript

1. EFFECTIVE CODE REVIEW
2. JUSTIN SPAHR-SUMMERS @JSPAHRSUMMERS
4. IT'S NOT SOFTWARE ENGINEERING WITHOUT CODE REVIEW
5. POOR QUALITY REVIEWS... > Waste everyone's time today > Waste everyone's time in the future > Provide a false sense of security
6. GREAT REVIEWS... > Minimize technical debt > Improve the architecture > Share domain knowledge > Provide teaching opportunities
7. FIRST PRINCIPLES
8. GOOD CODE REVIEW STARTS FROM THE SAME PERSPECTIVE AS WRITING GOOD CODE IF WE WERE TO WRITE THE BEST VERSION OF THIS, WHAT WOULD IT BE?
9. UNBLOCK OTHERS INCREASE YOUR TEAM'S PRODUCTIVITY
10. CRITIQUE THE CODE (NOT THE PERSON)
11. DON'T ASSUME IT'S OBVIOUS CODE CHANGES AND FEEDBACK BOTH NEED EXPLANATION
12. THE PRINCIPLES 1. Imagine the best version of the code 2. Unblock your team 3. Criticize code, not people 4. Over-explain everything!
13. THE PROCESS
14. START AT THE HIGHEST LEVEL, THEN DIVE DEEPER... 1. Intent Goal & summary 2. Design Architecture & API 3. Behavior Tests & implementation
15. 1. INTENT
16. WHAT IS THE GOAL? IS THE EXPLANATION CLEAR?
20. DOES IT SUCCEED? HOW DO YOU KNOW?
24. A GOOD SUMMARY SHOULD INCLUDE... > Background context > What the bug or feature is > What the change achieves > How the change is tested > Known limitations or anything still missing > Request changes if the summary is incomplete!
27. ARE THE CHANGES & SUMMARY COMPLETE?
28. 2. DESIGN
29. ASK YOURSELF: HOW WOULD YOU DO IT?
30. REVIEW THE ARCHITECTURE (THE COMPONENTS AND HOW THEY RELATE TO ONE ANOTHER) > Are the architectural choices justified? > Do you understand it well enough to use or extend? > Would everyone else be happy to maintain this? > Does the architecture make your life easier?
31. REVIEW THE API (THE CONTRACT FOR USING EACH COMPONENT) > Is the API understandable? > Does the documentation teach the reader how to use it? > Is the API conventional?
32. IS THE DESIGN GOOD?
33. YAGNI YOU AREN'T GONNA NEED IT
34. EASY FAMILIAR OR APPROACHABLE SIMPLE FEWER CONCEPTS AND CONCERNS
35. PIT OF SUCCESS MAKE THE RIGHT THINGS EASY & THE WRONG THINGS POSSIBLE
36. SOLID PRINCIPLES > Single-responsibility principle each thing should have only one responsibility > Open–closed principle behavior should be extensible without modifying code > Liskov substitution principle types should be replaceable with subtypes > Interface segregation principle many specific interfaces are better than one über-interface > Dependency inversion principle depend upon abstractions, not concrete implementations
37. 3. BEHAVIOR
38. REVIEW THE TESTS > Tests are additional documentation > Tests should protect against regressions > Tests should validate the API contract > Tests need to be understandable > Are there missing tests? > You can request changes!
39. REVIEW THE IMPLEMENTATION > This is the least important part to review! > Be suspicious of convoluted code > Would you be able to debug this code? > DRY: Don't repeat yourself > Don't reinvent the wheel > Don't ignore linters and warnings
40. THE PROCESS 1. Intent Goal & summary 2. Design Architecture & API 3. Behavior Tests & implementation
41. OTHER PROTIPS™
42. GIVE EFFECTIVE FEEDBACK > Always request changes or accept > Be pragmatic for urgent changes > Solicit second opinions > Prioritize your feedback > Provide concrete suggestions > Explain the feedback
43. TELL A STORY WITH YOUR COMMITS > Each commit should build logically upon the previous > Clean up after yourself: git rebase -i hg histedit > Stack dependent changes
44. THE PRINCIPLES 1. Imagine the best version of the code 2. Unblock your team 3. Criticize code, not people 4. Over-explain everything! THE PROCESS 1. Intent Goal & summary 2. Design Architecture & API 3. Behavior Tests & implementation
45. QUESTIONS? Slides and notes available at: github.com/jspahrsummers/effective-code-review Thanks to Lightricks and Barak Yoresh for inviting me to speak, and to Christoph Nakazawa for sending me some great example PRs!