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

C7393b7ba7ec9c8890dd77d209fbb3c9?s=47 maltzj
March 27, 2018
420

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/

C7393b7ba7ec9c8890dd77d209fbb3c9?s=128

maltzj

March 27, 2018
Tweet

Transcript

  1. Jonathan Maltz maltz@yelp.com/@maltzj Protect Your Source Using Code Review to

    Improve Your Application Quality
  2. Yelp’s Mission Connecting people with great local businesses.

  3. None
  4. None
  5. None
  6. None
  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?
  8. Why do Code Review?

  9. Make Sure Code is Correct

  10. Make Sure Code is Well-Designed

  11. Share Knowledge

  12. Basic Code Review Outline

  13. 1. Build Context

  14. • You understand the goal of the change • You

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

    calling for backup as necessary ◦ Especially important at boundaries Failure Modes?
  16. Authors are just as important here

  17. How to do it?

  18. Protip: pre-filled templates

  19. 2. Evaluate Closer

  20. • High-level design feedback • Bug-finding + edge case coverage

    • Test coverage • Sharing new patterns What’s Success?
  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?
  22. 3. Step Back

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

  24. Turnaround-time matters

  25. Lisa Sam

  26. Lisa Sam

  27. Lisa Sam

  28. Lisa Sam

  29. Lisa Sam

  30. Lisa Sam

  31. Lisa Sam

  32. Lisa Sam

  33. Lisa Sam

  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?
  35. How to Give Feedback

  36. None
  37. • Discussion + collaboration • Individual ownership • People actively

    wanting to receive peer feedback What do you want to encourage?
  38. None
  39. Feedback that makes people feel worse

  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?
  41. None
  42. “This code is good. Let’s make it even better.”

  43. None
  44. 1. Start With Facts

  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.
  46. None
  47. Opinion == Facts

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

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

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

    JUnit tests.
  51. Link to External Resources

  52. 2. Invite a Discussion

  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?
  54. 3. Replace “You” with “We” or “Us”

  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?
  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.
  57. None
  58. None
  59. Rule of Three

  60. On a scale of 1-10, I care about this at

    a 5. What about you? What are your major concerns?
  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?
  62. Celebrate Good Stuff

  63. Finding “The Right” Code to Review

  64. None
  65. Feedback Types

  66. Blocking Feedback

  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?
  68. Non-Blocking Feedback

  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?
  70. None
  71. Get all code reviews forwarded to you

  72. None
  73. Risk 0 ∞

  74. Opportunity 0 ∞

  75. Opportunity 0 ∞ Risk 0 ∞ Don’t do them unless

    assigned
  76. Opportunity 0 ∞ Risk 0 ∞ Don’t do them unless

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

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

    assigned Review for blocking feedback Add non- blocking feedback Do you have a plan?
  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?
  80. None
  81. ∞ 0 Perceived authority Tech lead New hire

  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?
  83. None
  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
  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
  86. www.yelp.com/careers/ We're Hiring!

  87. Questions

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