Reviewing Code Review
Lee Stone
!
@leesto
1
Code Reviews and How To Improve Them.
Improving the process of improving
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
Fundamentally create smaller stories to work on
End of day
End of day ‘Fixed’ Review Times I want to avoid this
Encouraging others to do reviews & acting on feedback
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