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

Slide 1

Slide 1 text

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

Slide 2

Slide 2 text

Yelp’s Mission Connecting people with great local businesses.

Slide 3

Slide 3 text

No content

Slide 4

Slide 4 text

No content

Slide 5

Slide 5 text

No content

Slide 6

Slide 6 text

No content

Slide 7

Slide 7 text

● 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?

Slide 8

Slide 8 text

Why do Code Review?

Slide 9

Slide 9 text

Make Sure Code is Correct

Slide 10

Slide 10 text

Make Sure Code is Well-Designed

Slide 11

Slide 11 text

Share Knowledge

Slide 12

Slide 12 text

Basic Code Review Outline

Slide 13

Slide 13 text

1. Build Context

Slide 14

Slide 14 text

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

Slide 15

Slide 15 text

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

Slide 16

Slide 16 text

Authors are just as important here

Slide 17

Slide 17 text

How to do it?

Slide 18

Slide 18 text

Protip: pre-filled templates

Slide 19

Slide 19 text

2. Evaluate Closer

Slide 20

Slide 20 text

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

Slide 21

Slide 21 text

● 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?

Slide 22

Slide 22 text

3. Step Back

Slide 23

Slide 23 text

Could this be done easier/better/faster?

Slide 24

Slide 24 text

Turnaround-time matters

Slide 25

Slide 25 text

Lisa Sam

Slide 26

Slide 26 text

Lisa Sam

Slide 27

Slide 27 text

Lisa Sam

Slide 28

Slide 28 text

Lisa Sam

Slide 29

Slide 29 text

Lisa Sam

Slide 30

Slide 30 text

Lisa Sam

Slide 31

Slide 31 text

Lisa Sam

Slide 32

Slide 32 text

Lisa Sam

Slide 33

Slide 33 text

Lisa Sam

Slide 34

Slide 34 text

● 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?

Slide 35

Slide 35 text

How to Give Feedback

Slide 36

Slide 36 text

No content

Slide 37

Slide 37 text

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

Slide 38

Slide 38 text

No content

Slide 39

Slide 39 text

Feedback that makes people feel worse

Slide 40

Slide 40 text

● “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?

Slide 41

Slide 41 text

No content

Slide 42

Slide 42 text

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

Slide 43

Slide 43 text

No content

Slide 44

Slide 44 text

1. Start With Facts

Slide 45

Slide 45 text

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.

Slide 46

Slide 46 text

No content

Slide 47

Slide 47 text

Opinion == Facts

Slide 48

Slide 48 text

Static methods cannot be mocked in normal JUnit tests.

Slide 49

Slide 49 text

✅ Static methods cannot be mocked in normal JUnit tests.

Slide 50

Slide 50 text

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

Slide 51

Slide 51 text

Link to External Resources

Slide 52

Slide 52 text

2. Invite a Discussion

Slide 53

Slide 53 text

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?

Slide 54

Slide 54 text

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

Slide 55

Slide 55 text

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?

Slide 56

Slide 56 text

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.

Slide 57

Slide 57 text

No content

Slide 58

Slide 58 text

No content

Slide 59

Slide 59 text

Rule of Three

Slide 60

Slide 60 text

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

Slide 61

Slide 61 text

● 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?

Slide 62

Slide 62 text

Celebrate Good Stuff

Slide 63

Slide 63 text

Finding “The Right” Code to Review

Slide 64

Slide 64 text

No content

Slide 65

Slide 65 text

Feedback Types

Slide 66

Slide 66 text

Blocking Feedback

Slide 67

Slide 67 text

● 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?

Slide 68

Slide 68 text

Non-Blocking Feedback

Slide 69

Slide 69 text

● 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?

Slide 70

Slide 70 text

No content

Slide 71

Slide 71 text

Get all code reviews forwarded to you

Slide 72

Slide 72 text

No content

Slide 73

Slide 73 text

Risk 0 ∞

Slide 74

Slide 74 text

Opportunity 0 ∞

Slide 75

Slide 75 text

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

Slide 76

Slide 76 text

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

Slide 77

Slide 77 text

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

Slide 78

Slide 78 text

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

Slide 79

Slide 79 text

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?

Slide 80

Slide 80 text

No content

Slide 81

Slide 81 text

∞ 0 Perceived authority Tech lead New hire

Slide 82

Slide 82 text

● 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?

Slide 83

Slide 83 text

No content

Slide 84

Slide 84 text

● 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

Slide 85

Slide 85 text

● 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