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

Protecting Your Source: Using Code Review To Im...

maltzj
March 27, 2018
730

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. • 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?
  2. • You understand the goal of the change • You

    understand what is out of scope • You understand any high-level decisions What’s Success?
  3. • Jumping into a code review right away • Not

    calling for backup as necessary ◦ Especially important at boundaries Failure Modes?
  4. • High-level design feedback • Bug-finding + edge case coverage

    • Test coverage • Sharing new patterns What’s Success?
  5. • 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?
  6. • 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?
  7. • Discussion + collaboration • Individual ownership • People actively

    wanting to receive peer feedback What do you want to encourage?
  8. • “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?
  9. 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.
  10. 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?
  11. 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?
  12. 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.
  13. On a scale of 1-10, I care about this at

    a 5. What about you? What are your major concerns?
  14. • 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?
  15. • 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?
  16. • 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?
  17. Opportunity 0 ∞ Risk 0 ∞ Don’t do them unless

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

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

    assigned Review for blocking feedback Add non- blocking feedback Do you have a plan?
  20. 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?
  21. • 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?
  22. • 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
  23. • 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