Upgrade to Pro — share decks privately, control downloads, hide ads and more …

Code Reviewing Like a Champion

maltzj
July 20, 2018
39k

Code Reviewing Like a Champion

These are the slides from my 360Andev Presentation on code review.

External Links:

Yelp's Code Review Guidelines: https://engineeringblog.yelp.com/2017/11/code-review-guidelines.html
How Square Writes Commit Messages: https://medium.com/square-corner-blog/how-square-writes-commit-messages-8e92fcbf77c9
How to Use Code Review To Execute Someone's Soul: https://www.daedtech.com/how-to-use-a-code-review-to-execute-someones-soul/
Creating a Strong Code Review Culture: https://www.youtube.com/watch?v=PJjmw9TRB7s
Honesty, Kindness, Inspiration: Pick Three: https://www.youtube.com/watch?v=hP_2XKYia9I
bettercode.reviews: http://www.bettercode.reviews/
Giving and Getting Technical Help: https://www.youtube.com/watch?v=hY14Er6JX2s
Crucial Conversations: https://www.amazon.com/Crucial-Conversations-Talking-Stakes-Second/dp/0071771328/ref=sr_1_3?ie=UTF8&qid=1521932464&sr=8-3&keywords=crucial+conversations
Pre-commit: https://pre-commit.com/

maltzj

July 20, 2018
Tweet

Transcript

  1. Code Reviewing Like A
    Champion
    Jonathan Maltz - @maltzj

    View full-size slide

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

    View full-size slide

  3. @maltzj
    Why do Code
    Review?

    View full-size slide

  4. @maltzj
    Make Sure Code
    is Correct

    View full-size slide

  5. @maltzj
    Make Sure Code
    is Well-Designed

    View full-size slide

  6. @maltzj
    Share Knowledge

    View full-size slide

  7. @maltzj
    Basic Code
    Review Outline

    View full-size slide

  8. @maltzj
    1. Build Context

    View full-size slide

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

    View full-size slide

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

    View full-size slide

  11. @maltzj
    Authors are just
    as important here

    View full-size slide

  12. @maltzj
    Protip: pre-filled
    templates

    View full-size slide

  13. @maltzj
    2. Evaluate
    Closer

    View full-size slide

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

    View full-size slide

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

    View full-size slide

  16. @maltzj
    3. Take a Step
    Back

    View full-size slide

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

    View full-size slide

  18. @maltzj
    How to Give
    Feedback

    View full-size slide

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

    View full-size slide

  20. @maltzj
    Feedback that
    makes people
    feel worse

    View full-size slide

  21. @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?

    View full-size slide

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

    View full-size slide

  23. @maltzj
    1. Start With
    Facts

    View full-size slide

  24. @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.”

    View full-size slide

  25. @maltzj
    Opinion == Facts

    View full-size slide

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

    View full-size slide

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

    View full-size slide

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

    View full-size slide

  29. @maltzj
    Link to External
    Resources

    View full-size slide

  30. @maltzj
    2. Invite a
    Discussion

    View full-size slide

  31. @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?”

    View full-size slide

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

    View full-size slide

  33. @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?”

    View full-size slide

  34. @maltzj
    Rule of Three

    View full-size slide

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

    View full-size slide

  36. @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?

    View full-size slide

  37. @maltzj
    Celebrate the
    Good Stuff

    View full-size slide

  38. @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

    View full-size slide