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

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