Code reviews and how to improve them

A presentation at DrupalCamp Bristol 2017 in July 2017 in Bristol, UK by Lee Stone

Slide 1

Slide 1

Reviewing Code Review Lee Stone
! @leesto 1 Code Reviews and How To Improve Them. Improving the process of improving

Slide 2

Slide 2

Reviewing Code Review Lee Stone
! @leesto 2 Reviewing Code Review. Improving the process of improving

Slide 3

Slide 3

Reviewing Code Review Lee Stone
! @leesto 3 Hi. I’m Lee.  Organiser of PHPSW One of a great team to make sure meetups happen

Web Team Lead at Gradwell Lead a team producing web applications for selling & managing Gradwell services $ Theatre Technician Local Amdram, Glastonbury, edFringe

Slide 4

Slide 4

Reviewing Code Review Lee Stone
! @leesto 4 What is Code Review?

Slide 5

Slide 5

Reviewing Code Review Lee Stone
! @leesto 5 The activity of checking another developer’s code for defects and improvement opportunities

Slide 6

Slide 6

Reviewing Code Review Lee Stone
! @leesto 6 Checking code through a dedicated tool prior to merge upstream

Slide 7

Slide 7

Reviewing Code Review Lee Stone
! @leesto 7 Why Should We Review Code?

Slide 8

Slide 8

Reviewing Code Review Lee Stone
! @leesto 8 Prevent Bugs Reaching Production. Security Business Logic Coding Standards

Slide 9

Slide 9

Reviewing Code Review Lee Stone
! @leesto 9 Knowledge Share Ideal to mentor new developers Shared business logic - Remove SPOFs Discover Technical Approaches

Slide 10

Slide 10

Reviewing Code Review Lee Stone
! @leesto 10 Other Benefits. Improve Estimates Making Sure Solutions Meet Requirements

Slide 11

Slide 11

Reviewing Code Review Lee Stone
! @leesto 11 What Code Review Isn’t

Slide 12

Slide 12

Reviewing Code Review Lee Stone
! @leesto 12 It does not replace training

Slide 13

Slide 13

Reviewing Code Review Lee Stone
! @leesto 13 It does not replace demos

Slide 14

Slide 14

Reviewing Code Review Lee Stone
! @leesto 14 My Environment

Slide 15

Slide 15

Reviewing Code Review Lee Stone
! @leesto 15 Service Delivery Web Dev Vo IP
team CE O, Offic e
Ma na ge r/ HR Meeting Room 3 Meeting Room 2 Finance Spar e des k s Info Assur PG Fi na nc e Info Assur CE O

Slide 16

Slide 16

Reviewing Code Review Lee Stone
! @leesto 15 Service Delivery Web Dev Vo IP
team CE O, Offic e
Ma na ge r/ HR Meeting Room 3 Meeting Room 2 Finance Spar e des k s Info Assur PG Fi na nc e Info Assur CE O

Slide 17

Slide 17

Reviewing Code Review Lee Stone
! @leesto 16

Slide 18

Slide 18

Reviewing Code Review Lee Stone
! @leesto 16 Senior Developer Vladimir Senior Developer Josip Mid-Level Developer Danijel Junior Developer Nebojsa Junior Developer Jovana QA Nemanja Junior Developer James Web Team Lead Lee

Slide 19

Slide 19

Reviewing Code Review Lee Stone
! @leesto 17 Our Tech Stack PHP Zend / Slim Frameworks Split Between Libraries / Applications Mercurial, not Git

Slide 20

Slide 20

Reviewing Code Review Lee Stone
! @leesto 18 How We Complete Code Reviews

Slide 21

Slide 21

Reviewing Code Review Lee Stone
! @leesto 19 Crucible. Links with Atlassian Suite See the reviews on Jira tickets ‘One Stop Shop’ for us Indicate Defects On a comment, can mark it is a defect which is shown separately Easily See Differences Between Commits Highlights added/removed lines Can drag between commits to see history Auto Update for Changes on Branch Developer doesn’t have to update review on making changes & can’t forget

Slide 22

Slide 22

Reviewing Code Review Lee Stone
! @leesto 19 Crucible. Links with Atlassian Suite See the reviews on Jira tickets ‘One Stop Shop’ for us Indicate Defects On a comment, can mark it is a defect which is shown separately Easily See Differences Between Commits Highlights added/removed lines Can drag between commits to see history Auto Update for Changes on Branch Developer doesn’t have to update review on making changes & can’t forget

Slide 23

Slide 23

Reviewing Code Review Lee Stone
! @leesto 19 Crucible. Links with Atlassian Suite See the reviews on Jira tickets ‘One Stop Shop’ for us Indicate Defects On a comment, can mark it is a defect which is shown separately Easily See Differences Between Commits Highlights added/removed lines Can drag between commits to see history Auto Update for Changes on Branch Developer doesn’t have to update review on making changes & can’t forget

Slide 24

Slide 24

Reviewing Code Review Lee Stone
! @leesto 19 Crucible. Links with Atlassian Suite See the reviews on Jira tickets ‘One Stop Shop’ for us Indicate Defects On a comment, can mark it is a defect which is shown separately Easily See Differences Between Commits Highlights added/removed lines Can drag between commits to see history Auto Update for Changes on Branch Developer doesn’t have to update review on making changes & can’t forget

Slide 25

Slide 25

Reviewing Code Review Lee Stone
! @leesto 19 Crucible. Links with Atlassian Suite See the reviews on Jira tickets ‘One Stop Shop’ for us Indicate Defects On a comment, can mark it is a defect which is shown separately Easily See Differences Between Commits Highlights added/removed lines Can drag between commits to see history Auto Update for Changes on Branch Developer doesn’t have to update review on making changes & can’t forget

Slide 26

Slide 26

Reviewing Code Review Lee Stone
! @leesto 20 Everything is Reviewed.

Slide 27

Slide 27

Reviewing Code Review Lee Stone
! @leesto 21 Everything Should be Reviewed Within 2 Days

Slide 28

Slide 28

Reviewing Code Review Lee Stone
! @leesto 22 Everyone is invited to the review

Slide 29

Slide 29

Reviewing Code Review Lee Stone
! @leesto 22 Everyone is invited to the review 1 x Senior must sign off

Slide 30

Slide 30

Reviewing Code Review Lee Stone
! @leesto 22 Everyone is invited to the review 1 x Senior must sign off 1 x Other must sign off

Slide 31

Slide 31

Reviewing Code Review Lee Stone
! @leesto 22 Everyone is invited to the review 1 x Senior must sign off 1 x Other must sign off No defects

Slide 32

Slide 32

Reviewing Code Review Lee Stone
! @leesto 23 HipChat Bot Because we all forget

Slide 33

Slide 33

Reviewing Code Review Lee Stone
! @leesto 24 Effective Code Reviews

Slide 34

Slide 34

Reviewing Code Review Lee Stone
! @leesto 25 Don’t Make it Personal.

Slide 35

Slide 35

Reviewing Code Review Lee Stone
! @leesto 25 Don’t Take it Personally.

Slide 36

Slide 36

Reviewing Code Review Lee Stone
! @leesto 26 I find using “we” rather than “I” or “You” helps

Slide 37

Slide 37

Reviewing Code Review Lee Stone
! @leesto 27 Don’t just point out the error or problem and move on Provide Solutions

Slide 38

Slide 38

Reviewing Code Review Lee Stone
! @leesto 28 Put whitespace only changes into separate commits

Slide 39

Slide 39

Reviewing Code Review Lee Stone
! @leesto 29 Put ‘library’ code and ‘application’ code into separate reviews

Slide 40

Slide 40

Reviewing Code Review Lee Stone
! @leesto 30 What Has Gone Well For Us?

Slide 41

Slide 41

Reviewing Code Review Lee Stone
! @leesto 31 Increased knowledge of similar work at sprint planning sessions.

Slide 42

Slide 42

Reviewing Code Review Lee Stone
! @leesto 32 Tended towards standard approaches in a quicker fashion.

Slide 43

Slide 43

Reviewing Code Review Lee Stone
! @leesto 33 Seen opportunities to refactor and produce more generic libraries earlier.

Slide 44

Slide 44

Reviewing Code Review Lee Stone
! @leesto 34 Caught Security Issues Before Hitting Production SQL Injection Modifying Other Customer’s Settings Producing a List of Customer Emails Partners Seeing Gradwell Only Pages

Slide 45

Slide 45

Reviewing Code Review Lee Stone
! @leesto 35 Improved In-line Documentation

Slide 46

Slide 46

Reviewing Code Review Lee Stone
! @leesto 36 The Business is Completely on-board

Slide 47

Slide 47

Reviewing Code Review Lee Stone
! @leesto 37 Increased Confidence From the Rest of the Business

Slide 48

Slide 48

Reviewing Code Review Lee Stone
! @leesto 38 Given Individual Developers More Freedom

Slide 49

Slide 49

Reviewing Code Review Lee Stone
! @leesto 39 Where Do I Have To Encourage The Team?

Slide 50

Slide 50

Reviewing Code Review Lee Stone
! @leesto 40 What are the most difficult reviews to get feedback on?

Slide 51

Slide 51

Reviewing Code Review Lee Stone
! @leesto 41 Anything by This Guy

Slide 52

Slide 52

Reviewing Code Review Lee Stone
! @leesto 42 A new developer to the team is more cautious about commenting on anyone else’s code. How to Encourage Newer Developers.

Slide 53

Slide 53

Reviewing Code Review Lee Stone
! @leesto 42 A new developer to the team is more cautious about commenting on anyone else’s code. How to Encourage Newer Developers. %

Start Small. Typos and coding styles are still important and are ‘less risky’ to comment on.

Slide 54

Slide 54

Reviewing Code Review Lee Stone
! @leesto 42 A new developer to the team is more cautious about commenting on anyone else’s code. How to Encourage Newer Developers. %

Start Small. Typos and coding styles are still important and are ‘less risky’ to comment on. %

Ask Why. Code reviews don’t have to be just for pointing out problems. Asking why an approach was taken can be a good learning opportunity.

Slide 55

Slide 55

Reviewing Code Review Lee Stone
! @leesto 42 A new developer to the team is more cautious about commenting on anyone else’s code. How to Encourage Newer Developers. %

Start Small. Typos and coding styles are still important and are ‘less risky’ to comment on. %

Ask Why. Code reviews don’t have to be just for pointing out problems. Asking why an approach was taken can be a good learning opportunity. %

Say If You Don’t Understand. We all have to maintain the code and readability is one of our most important considerations. If you don’t understand what is happening, we have probably done something wrong. Even if the code works.

Slide 56

Slide 56

Reviewing Code Review Lee Stone
! @leesto 42 A new developer to the team is more cautious about commenting on anyone else’s code. How to Encourage Newer Developers. %

Start Small. Typos and coding styles are still important and are ‘less risky’ to comment on. %

Ask Why. Code reviews don’t have to be just for pointing out problems. Asking why an approach was taken can be a good learning opportunity. %

Say If You Don’t Understand. We all have to maintain the code and readability is one of our most important considerations. If you don’t understand what is happening, we have probably done something wrong. Even if the code works. %

Bring Fresh Ideas Everyone can bring different experiences and suggest new and improved ways of approaching something.

Slide 57

Slide 57

Reviewing Code Review Lee Stone
! @leesto 43 Move Beyond Coding Standards

Slide 58

Slide 58

Reviewing Code Review Lee Stone
! @leesto 43 Move Beyond Coding Standards

Slide 59

Slide 59

Reviewing Code Review Lee Stone
! @leesto 44 Don’t stick to giving the same feedback Especially on a ‘minor benefit’ point

Slide 60

Slide 60

Reviewing Code Review Lee Stone
! @leesto 45 What Do We Need To Improve?

Slide 61

Slide 61

Reviewing Code Review Lee Stone
! @leesto 46 Introduce Code Coverage Reports

Slide 62

Slide 62

Reviewing Code Review Lee Stone
! @leesto 47 Get Reviews Completed Quicker

Slide 63

Slide 63

Reviewing Code Review Lee Stone
! @leesto 48

Slide 64

Slide 64

Reviewing Code Review Lee Stone
! @leesto 48

Slide 65

Slide 65

Reviewing Code Review Lee Stone
! @leesto 49 Why so Long? Reviewers Don’t Want to Interrupt Flow Other developers don’t look until they have finished their task, or at an ideal break point

Slide 66

Slide 66

Reviewing Code Review Lee Stone
! @leesto 49 Why so Long? Reviewers Don’t Want to Interrupt Flow Other developers don’t look until they have finished their task, or at an ideal break point Developers Move on to Something New Resistant to context switch back to make any suggested changes

Slide 67

Slide 67

Reviewing Code Review Lee Stone
! @leesto 49 Why so Long? Reviewers Don’t Want to Interrupt Flow Other developers don’t look until they have finished their task, or at an ideal break point Developers Move on to Something New Resistant to context switch back to make any suggested changes Large Reviews are Harder to Start People see ’67 files changed’ and delay

Slide 68

Slide 68

Reviewing Code Review Lee Stone
! @leesto 49 Why so Long? Reviewers Don’t Want to Interrupt Flow Other developers don’t look until they have finished their task, or at an ideal break point Developers Move on to Something New Resistant to context switch back to make any suggested changes Large Reviews are Harder to Start People see ’67 files changed’ and delay It Gets Worse The more cycles, the longer since the code was written, the longer it takes to switch

Slide 69

Slide 69

Reviewing Code Review Lee Stone
! @leesto 50 These Delays Hurt Our Release Pace

Slide 70

Slide 70

Reviewing Code Review Lee Stone
! @leesto 51 How to Speed Up? Encourage Smaller Reviews

More partial reviews early on

Fundamentally create smaller stories to work on

Slide 71

Slide 71

Reviewing Code Review Lee Stone
! @leesto 51 How to Speed Up? Encourage Smaller Reviews

More partial reviews early on

Fundamentally create smaller stories to work on Encourage Use of Small Time Periods

Before / after meetings

Before / after lunch.

End of day

Slide 72

Slide 72

Reviewing Code Review Lee Stone
! @leesto 51 How to Speed Up? Encourage Smaller Reviews

More partial reviews early on

Fundamentally create smaller stories to work on Encourage Use of Small Time Periods

Before / after meetings

Before / after lunch.

End of day ‘Fixed’ Review Times I want to avoid this

Slide 73

Slide 73

Reviewing Code Review Lee Stone
! @leesto 52 How to Speed Up…. Completion? Ownership of Work

Although we work as a team, we need a ‘champion’

Encouraging others to do reviews & acting on feedback

Slide 74

Slide 74

Reviewing Code Review Lee Stone
! @leesto 52 How to Speed Up…. Completion? Ownership of Work

Although we work as a team, we need a ‘champion’

Encouraging others to do reviews & acting on feedback Increase Recognition of Shipping

Verbal Recognition

How to handle for a remote team?

Slide 75

Slide 75

Reviewing Code Review Lee Stone
! @leesto 53 More Business Logic Feedback

Slide 76

Slide 76

Reviewing Code Review Lee Stone
! @leesto 54 Lets Recap

Slide 77

Slide 77

Reviewing Code Review Lee Stone
! @leesto 55 Code reviews take time, but bring benefits to teams. Your approach needs to be customised to suit your team. In Summary…

Slide 78

Slide 78

Reviewing Code Review Lee Stone
! @leesto 55 Code reviews take time, but bring benefits to teams. Your approach needs to be customised to suit your team. In Summary… &

Think About…. What Do You Want To Get Out Of The Reviews?

Slide 79

Slide 79

Reviewing Code Review Lee Stone
! @leesto 55 Code reviews take time, but bring benefits to teams. Your approach needs to be customised to suit your team. In Summary… &

Think About…. What Do You Want To Get Out Of The Reviews? &

Think About…. How Do You Get Buy In From The Team?

Slide 80

Slide 80

Reviewing Code Review Lee Stone
! @leesto 55 Code reviews take time, but bring benefits to teams. Your approach needs to be customised to suit your team. In Summary… &

Think About…. What Do You Want To Get Out Of The Reviews? &

Think About…. How Do You Get Buy In From The Team? &

Think About…. What Tooling Can Fit Into Your Workflow?

Slide 81

Slide 81

Reviewing Code Review Lee Stone
! @leesto 55 Code reviews take time, but bring benefits to teams. Your approach needs to be customised to suit your team. In Summary… &

Think About…. What Do You Want To Get Out Of The Reviews? &

Think About…. How Do You Get Buy In From The Team? &

Think About…. What Tooling Can Fit Into Your Workflow? &

Think About…. How Do You Not Considerably Slow Your Release Pace?

Slide 82

Slide 82

Reviewing Code Review Lee Stone
! @leesto 56 Thank You. Any Questions?

Slide 83

Slide 83

Reviewing Code Review Lee Stone
! @leesto 57 https://joind.in/talk/e62d8