$30 off During Our Annual Pro Sale. View Details »

Effective Code Review

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.

Justin Spahr-Summers

May 13, 2021
Tweet

More Decks by Justin Spahr-Summers

Other Decks in Programming

Transcript

  1. EFFECTIVE CODE REVIEW

  2. JUSTIN SPAHR-SUMMERS @JSPAHRSUMMERS

  3. None
  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?

  17. None
  18. None
  19. None
  20. DOES IT SUCCEED? HOW DO YOU KNOW?

  21. None
  22. None
  23. None
  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!
  25. None
  26. None
  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!