Slide 1

Slide 1 text

@jcinnamond THE ART OF CODE REVIEW

Slide 2

Slide 2 text

Code review is weird

Slide 3

Slide 3 text

We usually value… Collaboration Teamwork Removing distractions

Slide 4

Slide 4 text

You write some code Someone else comes along and points out all your mistakes

Slide 5

Slide 5 text

S talkhub Talks Issues Pull requests Settings Actually, `Collaboration` and `Teamwork` are saying the same thing search This talk

Slide 6

Slide 6 text

Code reviews are… Confrontational Egoistic Expensive Disruptive

Slide 7

Slide 7 text

REVIEWS VS HUGS 0 75 150 225 300 REVIEWS

Slide 8

Slide 8 text

So why bother?

Slide 9

Slide 9 text

Studies show code review can… Detect bugs Save money Improve communication Prevent global warming Source: some company selling code review software

Slide 10

Slide 10 text

If you're going to use code review… …use it well

Slide 11

Slide 11 text

trunk feature Pull Request

Slide 12

Slide 12 text

John's Top Tips for Great Code Review Joy Treat with a healthy dose of scepticism

Slide 13

Slide 13 text

Motivations

Slide 14

Slide 14 text

Expectations, Outcomes, and Challenges of Modern Code Review Alberto Bacchelli & Christian Bird (2013) International Conference on Software Engineering, pages 712-721

Slide 15

Slide 15 text

"What are the motivations and expectations for modern code review?"

Slide 16

Slide 16 text

DEVELOPERS' MOTIVATIONS FINDING DEFECTS CODE IMPROVEMENT ALTERNATIVE SOLUTIONS KNOWLEDGE TRANSFER 0 100 200 300 400

Slide 17

Slide 17 text

John's Top Tip #1 Discuss the motivation as a team

Slide 18

Slide 18 text

CODE IMPROVEMENT UNDERSTANDING SOCIAL COMMUNICATION FINDING DEFECTS 0% 12.5% 25% 37.5% 50% Motivations Outcomes

Slide 19

Slide 19 text

John's Top Tip #2 Compare actual outcome with motivations

Slide 20

Slide 20 text

Creating a pull request

Slide 21

Slide 21 text

Think about the reviewer Do they know what I'm trying to achieve? Are they aware of any constraints?

Slide 22

Slide 22 text

John's Top Tip #3 Write a description

Slide 23

Slide 23 text

In the description… Explain what the new feature is. Explain why you made it. Suggest a path through the code.

Slide 24

Slide 24 text

John's Top Tip #4 Keep change size < 400 lines Source: Cohen, Jason. (2006): The Best Kept Secrets of Peer Code Review.

Slide 25

Slide 25 text

Automated testing

Slide 26

Slide 26 text

John's Top Tip #5 Automatically run the test suite

Slide 27

Slide 27 text

John's Top Tip #6 Run linting tools (e.g., rubocop, hound)

Slide 28

Slide 28 text

Who should review?

Slide 29

Slide 29 text

To maximise defect detection, use 2 reviewers Source: Rigby, Peter C, and Bird, Christian. (2013): Convergent Contemporary Software Peer Review Practices.

Slide 30

Slide 30 text

Who should review? Only the lead developer (Gatekeeper)

Slide 31

Slide 31 text

Who should review? Only senior developers (Gatekeepers, but more of them)

Slide 32

Slide 32 text

Who should review? Developers at a similar level (True peer review)

Slide 33

Slide 33 text

Who should review? Everyone

Slide 34

Slide 34 text

John's Top Tip #7 Get everyone involved in reviews (even junior developers)

Slide 35

Slide 35 text

Reviewing code

Slide 36

Slide 36 text

Remember your motivation for reviewing

Slide 37

Slide 37 text

Valid comments (1) Problems with the code. i.e., actual defects (not just things you don't like)

Slide 38

Slide 38 text

John's Top Tip #8 Explain the problem, don't criticise the code

Slide 39

Slide 39 text

Valid comments (2) Questions about the code "I'm not sure what this does"

Slide 40

Slide 40 text

S talkhub Talks Issues Pull requests Settings That isn't a question? search This talk

Slide 41

Slide 41 text

Beware passive- aggressive questions Why did you use X here? Why did you use X here - are you some kind of idiot?

Slide 42

Slide 42 text

Valid comments (2) Questions about the code "WWSMD?"

Slide 43

Slide 43 text

Valid comments (3) Improvements to the code "map would be better than each"

Slide 44

Slide 44 text

John's Top Tip #9 Suggest improvements, don't dictate them

Slide 45

Slide 45 text

John's Top Tip #10 Justify your suggested improvements

Slide 46

Slide 46 text

Valid comments (4) Praise "this is nice! I'm going to use this…"

Slide 47

Slide 47 text

Valid comments (5) Inconsistency with the rest of the codebase

Slide 48

Slide 48 text

Valid comments (6) Coding standard violations But only if there is a standard to violate

Slide 49

Slide 49 text

John's Top Tip #11 Separate discussions on standards from code review

Slide 50

Slide 50 text

Valid comments (7) There are no other valid comments. Code review is not the place for this discussion.

Slide 51

Slide 51 text

Conversation is difficult via PR comments

Slide 52

Slide 52 text

John's Top Tip #13 Stop commenting. Talk.

Slide 53

Slide 53 text

John's Top Tip #14 Be yourself (unless you are a monstrous pedant, in which case be someone better)

Slide 54

Slide 54 text

Example

Slide 55

Slide 55 text

def any_values?(hash) return hash.values.size >= 0 end

Slide 56

Slide 56 text

def any_values?(hash) return hash.values.size >= 0 end This is dumb Doesn't explain the problem Feels bad, man

Slide 57

Slide 57 text

def any_values?(hash) return hash.values.size >= 0 end Never use return ☹ Why not? I'm being told off

Slide 58

Slide 58 text

def any_values?(hash) return hash.values.size >= 0 end Return is unnecessary Maybe I should change it Your face is unnecessary

Slide 59

Slide 59 text

def any_values?(hash) return hash.values.size >= 0 end This is equivalent to omitting the `return` The return is unnecessary

Slide 60

Slide 60 text

☹ def any_values?(hash) return hash.values.size >= 0 end The comparison is wrong I did a wrong Why?

Slide 61

Slide 61 text

def any_values?(hash) return hash.values.size >= 0 end I think you mean '> 0' Oops Go team!

Slide 62

Slide 62 text

def any_values?(hash) return hash.values.size >= 0 end This whole method is pointless My work is pointless

Slide 63

Slide 63 text

def any_values?(hash) return hash.values.size >= 0 end What is the point of this method? Why are they asking?

Slide 64

Slide 64 text

def any_values?(hash) return hash.values.size >= 0 end Why not ditch this method and call `hash.values.any?` directly? …

Slide 65

Slide 65 text

def any_values?(hash) return hash.values.size >= 0 end

Slide 66

Slide 66 text

The way you comment has a big impact

Slide 67

Slide 67 text

John's Top Tip #15 Avoid value judgements, even if they about the code

Slide 68

Slide 68 text

Responding to comments

Slide 69

Slide 69 text

Try to address every comment

Slide 70

Slide 70 text

Try to create separate commits for each problem found

Slide 71

Slide 71 text

Tell the reviewer when you have addressed everything

Slide 72

Slide 72 text

Managing disagreement

Slide 73

Slide 73 text

John's Top Tip #16 If you disagree with any comments talk about it

Slide 74

Slide 74 text

Don't reply with a new comment No good will come from this

Slide 75

Slide 75 text

John's Top Tip #17 If you still disagree after talking, ask someone else

Slide 76

Slide 76 text

Code review is inherently social Being "right" is not the most important thing

Slide 77

Slide 77 text

Recap

Slide 78

Slide 78 text

Decide what you want from Code Review, as a team 1

Slide 79

Slide 79 text

Check that it actually delivers those benefits 2

Slide 80

Slide 80 text

Don't be shits to each other in pull requests 3

Slide 81

Slide 81 text

Thank you The Art of Code Review @jcinnamond LRUG 2016