Code reviews and how to improve them

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Reviewing Code Review Lee Stone
! @leesto 14 My Environment

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

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

Reviewing Code Review Lee Stone
! @leesto 16

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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.

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.

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.

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.

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.

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

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

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

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

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

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

Reviewing Code Review Lee Stone
! @leesto 48

Reviewing Code Review Lee Stone
! @leesto 48

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

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

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

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

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

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

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

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

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

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?

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

Reviewing Code Review Lee Stone
! @leesto 54 Lets Recap

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…

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?

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?

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?

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?

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

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

Lee Stone
@leesto

1 / 83

We know that code reviews are considered an important part of creating high quality software and to making ourselves and our teams better - some of us even do them! Why are they beneficial though, what pitfalls are there and how could the process be improved?

Lee talks about some of his experiences with the code review phase of the development process and the steps he is taking to try and make it more informative, smoother and quicker for the entire team.