Code Reviewing Like a Champion

Code Reviewing Like A Champion Jonathan Maltz - @maltzj

@maltzj What Will We Talk About? • Why Do Code
Review • Basic Code Review Outline • How to Give Code Review Feedback

@maltzj Why do Code Review?

@maltzj Make Sure Code is Correct

@maltzj Make Sure Code is Well-Designed

@maltzj Share Knowledge

@maltzj Basic Code Review Outline

@maltzj 1. Build Context

@maltzj What’s Success? • You understand the goal of the
change • You understand what is out of scope • You understand any high-level decisions

@maltzj What’s Failure? • Jumping into a code review right
away • Not calling for backup as necessary ◦ Especially important at boundaries

@maltzj Authors are just as important here

@maltzj

@maltzj Protip: pre-filled templates

@maltzj 2. Evaluate Closer

@maltzj What’s Success? • High-level design feedback • Bug-finding +
edge case coverage • Test coverage • Naming • Sharing new patterns

@maltzj What’s Failure? • Commenting on anything a tool can
catch ◦ PMD ◦ Checkstyle ◦ Findbugs ◦ Pre-commit hooks • Discussing the same things repeatedly

@maltzj 3. Take a Step Back

@maltzj Could this be done easier/better/faster?

@maltzj How to Give Feedback

@maltzj • Discussion and collaboration • Authors owning their changes
throughout the process • People being excited about receiving feedback What Do You Want To Encourage?

@maltzj

@maltzj Feedback that makes people feel worse

@maltzj • “Your style here is inconsistent. Align your braces
with our code style.” • “This has a bug when running on Lollipop which will cause a crash. Fix it.” • “Why did you do this refactor? The code was better in its previous form.” How Can That Happen?

@maltzj

@maltzj “This code is good. Let’s make it even better.”

@maltzj 1. Start With Facts

@maltzj “The style guidelines say you should put braces on
the same line and this code puts braces on a new line. Align your braces with the styleguide.”

@maltzj

@maltzj Opinion == Facts

@maltzj Static methods cannot be mocked in normal JUnit tests.

@maltzj Static methods are bad. They cannot be mocked in
normal JUnit tests.

@maltzj Link to External Resources

@maltzj 2. Invite a Discussion

@maltzj “The style guidelines say you should put braces on
the same line and this code puts braces on a new line. Can you align your braces with the styleguide?”

@maltzj 3. Replace “You” with “We” or “Us”

@maltzj “The style guidelines say we should put braces on
the same line and this code puts braces on a new line. Can we align the braces with our styleguide?”

@maltzj

@maltzj Rule of Three

@maltzj “On a scale of 1-10 I care about this
at about a 5. What about you? What are your major concerns?”

@maltzj • Different concerns? Find a third way • Large
Difference? Principle of the person who cares the most in the code review ◦ Protip: This shouldn’t always be you • Both high? Maybe need a larger decision What to do with this?

@maltzj Celebrate the Good Stuff

@maltzj

@maltzj External Links • Yelp’s Code Review Guidelines • Writing
Commit Messages • How to Use Code Review To Execute Someone’s Soul • Creating a Strong Code Review Culture • Honesty Kindness, Inspiration: Pick Three • Bettercode.reviews • Giving and Getting Technical Help • Crucial Conversations • Pre-commit

Code Reviewing Like a Champion

Code Reviewing Like a Champion

More Decks by maltzj

Featured

Transcript