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

Protecting Your Source: Using Code Review To Improve Your Application Quality (Droidcon Boston 2018)

maltzj
March 27, 2018
630

Protecting Your Source: Using Code Review To Improve Your Application Quality (Droidcon Boston 2018)

This is my talk on using code review to improve your application quality from Droidcon Boston.

Useful 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

March 27, 2018
Tweet

Transcript

  1. Jonathan Maltz
    [email protected]/@maltzj
    Protect Your Source
    Using Code Review to Improve Your Application
    Quality

    View Slide

  2. Yelp’s Mission
    Connecting people with great
    local businesses.

    View Slide

  3. View Slide

  4. View Slide

  5. View Slide

  6. View Slide

  7. ● Why do Code Review
    ● Basic Code Review Outline
    ● How to give code review feedback
    ● How to find the right code to review
    What Will We Talk About?

    View Slide

  8. Why do Code Review?

    View Slide

  9. Make Sure Code is
    Correct

    View Slide

  10. Make Sure Code is
    Well-Designed

    View Slide

  11. Share Knowledge

    View Slide

  12. Basic Code Review
    Outline

    View Slide

  13. 1. Build Context

    View Slide

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

    View Slide

  15. ● Jumping into a code review right away
    ● Not calling for backup as necessary
    ○ Especially important at boundaries
    Failure Modes?

    View Slide

  16. Authors are just as
    important here

    View Slide

  17. How to do it?

    View Slide

  18. Protip: pre-filled
    templates

    View Slide

  19. 2. Evaluate Closer

    View Slide

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

    View Slide

  21. ● Commenting on anything a tool can catch
    ○ PMD
    ○ Checkstyle
    ○ Findbugs
    ○ Pre-commit hooks
    ○ Google Java format
    ● Can’t catch things with a tool but always disagree on
    them? Come to a consensus and move on
    What’s Failure?

    View Slide

  22. 3. Step Back

    View Slide

  23. Could this be done
    easier/better/faster?

    View Slide

  24. Turnaround-time
    matters

    View Slide

  25. Lisa
    Sam

    View Slide

  26. Lisa
    Sam

    View Slide

  27. Lisa
    Sam

    View Slide

  28. Lisa
    Sam

    View Slide

  29. Lisa
    Sam

    View Slide

  30. Lisa
    Sam

    View Slide

  31. Lisa
    Sam

    View Slide

  32. Lisa
    Sam

    View Slide

  33. Lisa
    Sam

    View Slide

  34. ● Be conscious of burying teammates in code reviews
    ● Try to get prioritize shipping 1 or 2 reviews before
    submitting more.
    ● If your teammates are being slow to turnaround code
    reviews, don’t submit more
    How to avoid?

    View Slide

  35. How to Give Feedback

    View Slide

  36. View Slide

  37. ● Discussion + collaboration
    ● Individual ownership
    ● People actively wanting to receive peer feedback
    What do you want to encourage?

    View Slide

  38. View Slide

  39. Feedback that
    makes people feel
    worse

    View Slide

  40. ● “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”
    What to avoid?

    View Slide

  41. View Slide

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

    View Slide

  43. View Slide

  44. 1. Start With Facts

    View Slide

  45. 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 our styleguide.

    View Slide

  46. View Slide

  47. Opinion == Facts

    View Slide

  48. Static methods
    cannot be mocked in
    normal JUnit tests.

    View Slide

  49. ✅ Static methods
    cannot be mocked in
    normal JUnit tests.

    View Slide

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

    View Slide

  51. Link to External
    Resources

    View Slide

  52. 2. Invite a
    Discussion

    View Slide

  53. 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 our
    styleguide?

    View Slide

  54. 3. Replace “You”
    with “We” or “Us”

    View Slide

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

    View Slide

  56. The style guidelines say we
    should put braces on the same
    line, and this code puts braces
    on a new line. Let’s align our
    braces with our styleguide.

    View Slide

  57. View Slide

  58. View Slide

  59. Rule of Three

    View Slide

  60. On a scale of 1-10, I care
    about this at a 5. What
    about you? What are
    your major concerns?

    View Slide

  61. ● 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 to make a larger decision
    What to do with this?

    View Slide

  62. Celebrate
    Good Stuff

    View Slide

  63. Finding “The Right” Code
    to Review

    View Slide

  64. View Slide

  65. Feedback Types

    View Slide

  66. Blocking Feedback

    View Slide

  67. ● Things that need to be fixed before shipping
    ○ Bugs, major code smells, lacking test coverage
    ● E.g: Kitkat doesn’t support elevation, which means this will
    crash on older devices. Can we use ViewCompat instead?
    What’s it look like?

    View Slide

  68. Non-Blocking
    Feedback

    View Slide

  69. ● Suggestions, ideas, or opportunities that may have been
    missed
    ○ Alternate design, renaming, opportunistic refactoring
    ● E.g: Not sure if this is a good idea, but what if we changed
    this code to use a builder pattern instead of some static
    factories?
    What’s it look like?

    View Slide

  70. View Slide

  71. Get all code reviews
    forwarded to you

    View Slide

  72. View Slide

  73. Risk
    0 ∞

    View Slide

  74. Opportunity
    0

    View Slide

  75. Opportunity
    0

    Risk
    0 ∞
    Don’t do
    them
    unless
    assigned

    View Slide

  76. Opportunity
    0

    Risk
    0 ∞
    Don’t do
    them
    unless
    assigned
    Review for
    blocking
    feedback

    View Slide

  77. Opportunity
    0

    Risk
    0 ∞
    Don’t do
    them
    unless
    assigned
    Review for
    blocking
    feedback
    Do you
    have a
    plan?

    View Slide

  78. Opportunity
    0

    Risk
    0 ∞
    Don’t do
    them
    unless
    assigned
    Review for
    blocking
    feedback
    Add non-
    blocking
    feedback
    Do you
    have a
    plan?

    View Slide

  79. Non-Blocking: This class
    has always felt a little
    clunky to me. Now that
    we’re in here, what if we
    clean it up a bit?

    View Slide

  80. View Slide


  81. 0
    Perceived
    authority
    Tech lead New hire

    View Slide

  82. ● Argue both sides of the case
    ● Call-out a problem without suggesting a solution
    ● Invite them to pair with you
    How to deal with this?

    View Slide

  83. View Slide

  84. ● Make sure to not overwhelm your teammates with code
    reviews
    ● Phrase your feedback as a question
    ● Don’t always be the person who cares the most
    Three Big Takeaways

    View Slide

  85. ● 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
    External Links

    View Slide

  86. www.yelp.com/careers/
    We're Hiring!

    View Slide

  87. Questions

    View Slide

  88. @YelpEngineering
    fb.com/YelpEngineers
    engineeringblog.yelp.com
    github.com/yelp

    View Slide