Effective Code Review - Speaker Deck

Slide 1

Slide 1 text

EFFECTIVE CODE REVIEW

Slide 2

Slide 2 text

JUSTIN SPAHR-SUMMERS @JSPAHRSUMMERS

Slide 3

Slide 3 text

No content

Slide 4

Slide 4 text

IT'S NOT SOFTWARE ENGINEERING WITHOUT CODE REVIEW

Slide 5

Slide 5 text

POOR QUALITY REVIEWS... > Waste everyone's time today > Waste everyone's time in the future > Provide a false sense of security

Slide 6

Slide 6 text

GREAT REVIEWS... > Minimize technical debt > Improve the architecture > Share domain knowledge > Provide teaching opportunities

Slide 7

Slide 7 text

FIRST PRINCIPLES

Slide 8

Slide 8 text

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?

Slide 9

Slide 9 text

UNBLOCK OTHERS INCREASE YOUR TEAM'S PRODUCTIVITY

Slide 10

Slide 10 text

CRITIQUE THE CODE (NOT THE PERSON)

Slide 11

Slide 11 text

DON'T ASSUME IT'S OBVIOUS CODE CHANGES AND FEEDBACK BOTH NEED EXPLANATION

Slide 12

Slide 12 text

THE PRINCIPLES 1. Imagine the best version of the code 2. Unblock your team 3. Criticize code, not people 4. Over-explain everything!

Slide 13

Slide 13 text

THE PROCESS

Slide 14

Slide 14 text

START AT THE HIGHEST LEVEL, THEN DIVE DEEPER... 1. Intent Goal & summary 2. Design Architecture & API 3. Behavior Tests & implementation

Slide 15

Slide 15 text

1. INTENT

Slide 16

Slide 16 text

WHAT IS THE GOAL? IS THE EXPLANATION CLEAR?

Slide 17

Slide 17 text

No content

Slide 18

Slide 18 text

No content

Slide 19

Slide 19 text

No content

Slide 20

Slide 20 text

DOES IT SUCCEED? HOW DO YOU KNOW?

Slide 21

Slide 21 text

No content

Slide 22

Slide 22 text

No content

Slide 23

Slide 23 text

No content

Slide 24

Slide 24 text

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!

Slide 25

Slide 25 text

No content

Slide 26

Slide 26 text

No content

Slide 27

Slide 27 text

ARE THE CHANGES & SUMMARY COMPLETE?

Slide 28

Slide 28 text

2. DESIGN

Slide 29

Slide 29 text

ASK YOURSELF: HOW WOULD YOU DO IT?

Slide 30

Slide 30 text

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?

Slide 31

Slide 31 text

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?

Slide 32

Slide 32 text

IS THE DESIGN GOOD?

Slide 33

Slide 33 text

YAGNI YOU AREN'T GONNA NEED IT

Slide 34

Slide 34 text

EASY FAMILIAR OR APPROACHABLE SIMPLE FEWER CONCEPTS AND CONCERNS

Slide 35

Slide 35 text

PIT OF SUCCESS MAKE THE RIGHT THINGS EASY & THE WRONG THINGS POSSIBLE

Slide 36

Slide 36 text

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

Slide 37

Slide 37 text

3. BEHAVIOR

Slide 38

Slide 38 text

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!

Slide 39

Slide 39 text

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

Slide 40

Slide 40 text

THE PROCESS 1. Intent Goal & summary 2. Design Architecture & API 3. Behavior Tests & implementation

Slide 41

Slide 41 text

OTHER PROTIPS™

Slide 42

Slide 42 text

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

Slide 43

Slide 43 text

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

Slide 44

Slide 44 text

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

Slide 45

Slide 45 text

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!