Pro Yearly is on sale from $80 to $50! »

The Art of Code Review

The Art of Code Review

A talk about Code Review, given at LRUG in May 2016

76c1414a09f42ad83b2296a8cdcd11a9?s=128

John Cinnamond

May 09, 2016
Tweet

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