The Art of Code Review

Transcript

1. @jcinnamond THE ART OF CODE REVIEW

2. Code review is weird

3. We usually value… Collaboration Teamwork Removing distractions

4. You write some code Someone else comes along and points

   out all your mistakes

5. S talkhub Talks Issues Pull requests Settings Actually, `Collaboration` and

   `Teamwork` are saying the same thing search This talk

6. Code reviews are… Confrontational Egoistic Expensive Disruptive

7. REVIEWS VS HUGS 0 75 150 225 300 REVIEWS

8. So why bother?

9. Studies show code review can… Detect bugs Save money Improve

   communication Prevent global warming Source: some company selling code review software

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

11. trunk feature Pull Request

12. John's Top Tips for Great Code Review Joy Treat with

    a healthy dose of scepticism

13. Motivations

14. Expectations, Outcomes, and Challenges of Modern Code Review Alberto Bacchelli

    & Christian Bird (2013) International Conference on Software Engineering, pages 712-721

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

16. DEVELOPERS' MOTIVATIONS FINDING DEFECTS CODE IMPROVEMENT ALTERNATIVE SOLUTIONS KNOWLEDGE TRANSFER

    0 100 200 300 400

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

18. CODE IMPROVEMENT UNDERSTANDING SOCIAL COMMUNICATION FINDING DEFECTS 0% 12.5% 25%

    37.5% 50% Motivations Outcomes

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

20. Creating a pull request

21. Think about the reviewer Do they know what I'm trying

    to achieve? Are they aware of any constraints?

22. John's Top Tip #3 Write a description

23. In the description… Explain what the new feature is. Explain

    why you made it. Suggest a path through the code.

24. John's Top Tip #4 Keep change size < 400 lines

    Source: Cohen, Jason. (2006): The Best Kept Secrets of Peer Code Review.

25. Automated testing

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

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

28. Who should review?

29. To maximise defect detection, use 2 reviewers Source: Rigby, Peter

    C, and Bird, Christian. (2013): Convergent Contemporary Software Peer Review Practices.

30. Who should review? Only the lead developer (Gatekeeper)

31. Who should review? Only senior developers (Gatekeepers, but more of

    them)

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

    review)

33. Who should review? Everyone

34. John's Top Tip #7 Get everyone involved in reviews (even

    junior developers)

35. Reviewing code

36. Remember your motivation for reviewing

37. Valid comments (1) Problems with the code. i.e., actual defects

    (not just things you don't like)

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

    code

39. Valid comments (2) Questions about the code "I'm not sure

    what this does"

40. S talkhub Talks Issues Pull requests Settings That isn't a

    question? search This talk

41. Beware passive- aggressive questions Why did you use X here?

    Why did you use X here - are you some kind of idiot?

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

43. Valid comments (3) Improvements to the code "map would be

    better than each"

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

45. John's Top Tip #10 Justify your suggested improvements

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

    use this…"

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

48. Valid comments (6) Coding standard violations But only if there

    is a standard to violate

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

    review

50. Valid comments (7) There are no other valid comments. Code

    review is not the place for this discussion.

51. Conversation is difficult via PR comments

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

53. John's Top Tip #14 Be yourself (unless you are a

    monstrous pedant, in which case be someone better)

54. Example

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

56. def any_values?(hash) return hash.values.size >= 0 end This is dumb

    Doesn't explain the problem Feels bad, man

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

    ☹ Why not? I'm being told off

58. def any_values?(hash) return hash.values.size >= 0 end Return is unnecessary

    Maybe I should change it Your face is unnecessary

59. def any_values?(hash) return hash.values.size >= 0 end This is equivalent

    to omitting the `return` The return is unnecessary

60. ☹ def any_values?(hash) return hash.values.size >= 0 end The comparison

    is wrong I did a wrong Why?

61. def any_values?(hash) return hash.values.size >= 0 end I think you

    mean '> 0' Oops Go team!

62. def any_values?(hash) return hash.values.size >= 0 end This whole method

    is pointless My work is pointless

63. def any_values?(hash) return hash.values.size >= 0 end What is the

    point of this method? Why are they asking?

64. def any_values?(hash) return hash.values.size >= 0 end Why not ditch

    this method and call `hash.values.any?` directly? …

65. def any_values?(hash) return hash.values.size >= 0 end <puts down pull

    request, talks to developer?

66. The way you comment has a big impact

67. John's Top Tip #15 Avoid value judgements, even if they

    about the code

68. Responding to comments

69. Try to address every comment

70. Try to create separate commits for each problem found

71. Tell the reviewer when you have addressed everything

72. Managing disagreement

73. John's Top Tip #16 If you disagree with any comments

    talk about it

74. Don't reply with a new comment No good will come

    from this

75. John's Top Tip #17 If you still disagree after talking,

    ask someone else

76. Code review is inherently social Being "right" is not the

    most important thing

77. Recap

78. Decide what you want from Code Review, as a team

    1

79. Check that it actually delivers those benefits 2

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

81. Thank you The Art of Code Review @jcinnamond LRUG 2016