Upgrade to Pro — share decks privately, control downloads, hide ads and more …

How To Do Positive Code Reviews

How To Do Positive Code Reviews

How to create an environment where code reviews are a positive experience for everyone involved.

Presented at PHP Cambridge on Feb 16th 2016.

2c1dc90ff7bf69097a151677624777d2?s=128

Stuart Herbert

February 16, 2016
Tweet

Transcript

  1. A presentation by @stuherbert
 for @GanbaroDigital Build A Culture
 Of

    Improvement Positive
 Code Reviews
  2. @GanbaroDigital Huge shoutout to Jo for asking me to do

    this talk.
  3. @GanbaroDigital Do you do code reviews?

  4. @GanbaroDigital Tell me about them.

  5. @GanbaroDigital How do you feel when your code is reviewed?

  6. @GanbaroDigital How do you feel when you review someone else’s

    code?
  7. @GanbaroDigital Why do you feel like that?

  8. @GanbaroDigital A positive code review is one that all participants

    actively want to take part in.
  9. @GanbaroDigital To understand code reviews, it helps to understand the

    process that they sit in.
  10. @GanbaroDigital How Tasks Are Typically Done 1. Developer is assigned

    ticket in JIRA 2. Developer writes code 
 (and maybe some tests) 3. Best mate reviews code if they have the time 4. Developer merges code
  11. @GanbaroDigital Let’s break that down.

  12. @GanbaroDigital How Tasks Are Typically Done 1. Developer is assigned

    ticket in JIRA 2. Developer writes code 
 (and maybe some tests) 3. Best mate reviews code if they have the time 4. Developer merges code
  13. @GanbaroDigital How many organisations create JIRA tickets that are ready

    to code?
  14. @GanbaroDigital How Tasks Are Typically Done 1. Developer is assigned

    ticket in JIRA 2. Developer writes code 
 (and maybe some tests) 3. Best mate reviews code if they have the time 4. Developer merges code
  15. @GanbaroDigital How many developers question what they are told to

    deliver?
  16. @GanbaroDigital How many developers have access to the person they

    need to question?
  17. @GanbaroDigital How Tasks Are Typically Done 1. Developer is assigned

    ticket in JIRA 2. Developer writes code 
 (and maybe some tests) 3. Best mate reviews code if they have the time 4. Developer merges code
  18. @GanbaroDigital How many organisations require tests alongside code?

  19. @GanbaroDigital How Tasks Are Typically Done 1. Developer is assigned

    ticket in JIRA 2. Developer writes code 
 (and maybe some tests) 3. Best mate reviews code if they have the time 4. Developer merges code
  20. @GanbaroDigital How many organisations define how many tests must accompany

    code?
  21. @GanbaroDigital How Tasks Are Typically Done 1. Developer is assigned

    ticket in JIRA 2. Developer writes code 
 (and maybe some tests) 3. Best mate reviews code if they have the time 4. Developer merges code
  22. @GanbaroDigital It is natural to seek out people who are

    more likely to agree with us. Especially in a toxic culture.
  23. @GanbaroDigital How many organisations require code reviews?

  24. @GanbaroDigital How many organisations define code review standards?

  25. @GanbaroDigital What standards do you review against?

  26. @GanbaroDigital PSR-2 is a formatting standard. It is not a

    complete code review standard.
  27. @GanbaroDigital How Tasks Are Typically Done 1. Developer is assigned

    ticket in JIRA 2. Developer writes code 
 (and maybe some tests) 3. Best mate reviews code if they have the time 4. Developer merges code
  28. @GanbaroDigital How many organisations know what is in their shipped

    software?
  29. @GanbaroDigital How Tasks Are Typically Done 1. Developer is assigned

    ticket in JIRA 2. Developer writes code 
 (and maybe some tests) 3. Best mate reviews code if they have the time 4. Developer merges code
  30. @GanbaroDigital Positive code reviews?!? Despite the culture, not because of

    it.
  31. @GanbaroDigital Unpleasant code reviews are a side-effect of organisation culture.

  32. @GanbaroDigital Unpleasant code reviews are a side-effect of a lack

    of deliberate organisation culture.
  33. @GanbaroDigital To fix your code reviews, you have to fix

    your culture.
  34. @GanbaroDigital Some cultures are too toxic to be fixed :-(

  35. @GanbaroDigital No process can overcome toxic people :-(

  36. @GanbaroDigital Culture is what happens when management leaves the room.

  37. @GanbaroDigital Culture is a reaction to management behaviour.

  38. @GanbaroDigital Leaders get the culture that they live themselves.

  39. @GanbaroDigital Be a better leader to have a better culture.

  40. @GanbaroDigital How does this help us do positive code reviews?

  41. @GanbaroDigital Introducing: the rule of no surprises.

  42. @GanbaroDigital Do not violate the rule of no surprises.

  43. @GanbaroDigital Set expectations up front.

  44. @GanbaroDigital Set measurable expectations up front.

  45. @GanbaroDigital Provide evidence for and against defined expectations and standards.

  46. @GanbaroDigital Provide evidence for and against agreed expectations and standards.

  47. @GanbaroDigital Provide evidence for and against agreed expectations and standards.

  48. @GanbaroDigital Leave opinion at the door.

  49. @GanbaroDigital Introducing: everything must stand up to scrutiny.

  50. @GanbaroDigital Everything == 1. Requirements 2. Design 3. Written code

    4. Test plan
  51. @GanbaroDigital Everything == 1. Requirements 2. Design 3. Written code

    4. Test plan
  52. @GanbaroDigital People do a better job when they believe in

    what they’re asked to do.
  53. @GanbaroDigital Everything == 1. Requirements 2. Design 3. Written code

    4. Test plan
  54. @GanbaroDigital Teams write better code when they’re contributing to a

    consistent design.
  55. @GanbaroDigital Everything == 1. Requirements 2. Design 3. Written code

    4. Test plan
  56. @GanbaroDigital Everything == 1. Requirements 2. Design 3. Written code

    4. Test plan
  57. @GanbaroDigital Review tests that cover the happy path.

  58. @GanbaroDigital Review tests that prove robustness.

  59. @GanbaroDigital Everything == 1. Requirements 2. Design 3. Written code

    4. Test plan
  60. @GanbaroDigital “Do as I say, not as I do.” How

    many of you have worked for someone like that?
  61. @GanbaroDigital Lead by example. Make sure all of your work

    is reviewed.
  62. @GanbaroDigital Lead by example. Make sure all of your work

    meets the review standards.
  63. @GanbaroDigital Only then do you have the moral right to

    review the work of your colleagues.
  64. @GanbaroDigital Introducing: everyone is entitled to a bad day.

  65. @GanbaroDigital It isn’t credible to expect everyone to be brilliant

    every day.
  66. @GanbaroDigital Introducing: ask, don’t tell

  67. @GanbaroDigital Introducing: ask, don’t tell (sometimes)

  68. @GanbaroDigital How We Learn 1. Instruction 2. Coaching 3. Collaboration

  69. @GanbaroDigital How We Learn 1. Instruction 2. Coaching 3. Collaboration

  70. @GanbaroDigital How We Learn 1. Instruction 2. Coaching 3. Collaboration

  71. @GanbaroDigital Not everyone is comfortable giving or receiving instruction.

  72. @GanbaroDigital Not everyone reaches the coaching stage.

  73. @GanbaroDigital Guiding Principles 1. The rule of no surprises. 2.

    Everything must stand up to scrutiny. 3. Everyone is entitled to a bad day. 4. Ask, don’t tell (sometimes).
  74. @GanbaroDigital Discussion: How do we incorporate these principles into a

    positive review process?
  75. @GanbaroDigital Change the conversation.

  76. @GanbaroDigital 1. Make sure only the right people attend. 2.

    Make sure expectations are known in advance. 3. Discover and correct problems early.
  77. @GanbaroDigital Code Review Attendees 1. Person doing the work. 2.

    Their boss - team lead or senior dev. 3. Observers who need to learn.
  78. @GanbaroDigital Code Review Attendees 1. Person doing the work. 2.

    Their boss - team lead or senior dev. 3. Observers who need to learn.
  79. @GanbaroDigital Code Review Attendees 1. Person doing the work. 2.

    Their boss - team lead or senior dev. 3. Observers who need to learn.
  80. @GanbaroDigital Review Thrice, Ship Once 1. Review task and plan

    of attack. 2. Review code. 3. Review & approve code + tests.
  81. @GanbaroDigital Review Thrice, Ship Once 1. Review task and plan

    of attack. 2. Review code. 3. Review & approve code + tests.
  82. @GanbaroDigital Before Writing Code • Check understanding of the problem.

    • Review proposed solution. • Review proposed test plan. • Give or withhold approval.
  83. @GanbaroDigital Review Thrice, Ship Once 1. Review task and plan

    of attack. 2. Review code. 3. Review & approve code + tests.
  84. @GanbaroDigital After Writing Code • Review developed solution. • Review

    revised test plan. • Give or withhold approval.
  85. @GanbaroDigital Review Thrice, Ship Once 1. Review task and plan

    of attack. 2. Review code. 3. Review & approve code + tests.
  86. @GanbaroDigital Before Merging • Repeat test plan execution. • Review

    test plan execution results. • Give or withhold approval.
  87. @GanbaroDigital Break down review points into general and specific items.

  88. @GanbaroDigital Make everyone 
 involved in a code review 


    sign off on quality.
  89. @GanbaroDigital Make everyone 
 involved in a code review 


    accountable for everything that they have approved.
  90. @GanbaroDigital Give everyone 
 involved in a code review 


    the right to refuse to sign off the work.
  91. @GanbaroDigital … as long as their refusal is evidence-based against

    documented standards & requirements!
  92. @GanbaroDigital Review Thrice, Ship Once 1. Review task and plan

    of attack. 2. Review code. 3. Review & approve code + tests.
  93. @GanbaroDigital In summary …

  94. @GanbaroDigital The root causes of unpleasant code reviews are to

    be found a long way upstream.
  95. @GanbaroDigital Agree review standards up front.

  96. @GanbaroDigital Evidence based, not opinion based.

  97. @GanbaroDigital Lead by example, and the right people will respond

    in kind.
  98. @GanbaroDigital Review thrice, merge once.

  99. @GanbaroDigital Make everyone 
 involved in a code review 


    sign off on quality.
  100. @GanbaroDigital No process can solve the problem of toxic people

    … … except perhaps the firing process!
  101. Would you like to 
 know more? We can help.

    Stuart Herbert ~ @stuherbert Founder @GanbaroDigital