A presentation at PHPSW New Year Coding Resolutions, January 2017 in in Bristol, UK by Lee Stone
Reviewing Code Review
Lee Stone
!
@leesto
1
Reviewing Code Review.
Improving the process of improving
Reviewing Code Review
Lee Stone
!
@leesto
2
Hi. I’m Lee.
Organiser of PHPSW
One of a great team to make sure nights like
tonight 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
3
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
3
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
4
Reviewing Code Review
Lee Stone
!
@leesto
4
Senior Developer
Stefan
Mid-Level Developer
Nemanja
Mid-Level Developer
Vladimir
Junior Developer
Danijel
Junior Developer
Jovana
QA
Nemanja
Junior Developer
James
Web Team Lead
Lee
Reviewing Code Review
Lee Stone
!
@leesto
5
What is Code Review?
Reviewing Code Review
Lee Stone
!
@leesto
6
The activity of checking another
developer’s code for defects and
improvement opportunities
Reviewing Code Review
Lee Stone
!
@leesto
7
Checking code through a dedicated tool
prior to merge upstream
Reviewing Code Review
Lee Stone
!
@leesto
8
Why Should We Review Code?
Reviewing Code Review
Lee Stone
!
@leesto
9
Prevent Bugs Reaching Production.
Security
Business Logic
Coding Standards
Reviewing Code Review
Lee Stone
!
@leesto
10
Knowledge Share
Ideal to mentor new developers
Shared business logic - Remove SPOFs
Discover Technical Approaches
Reviewing Code Review
Lee Stone
!
@leesto
11
Other Benefits.
Improve Estimates
Making Sure Solutions Meet Requirements
Reviewing Code Review
Lee Stone
!
@leesto
12
How We Complete Code Reviews
Reviewing Code Review
Lee Stone
!
@leesto
13
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
14
Everything is Reviewed.
Reviewing Code Review
Lee Stone
!
@leesto
15
Everything Should be Reviewed Within 2 Days
Reviewing Code Review
Lee Stone
!
@leesto
16
Everyone is invited to the review
Reviewing Code Review
Lee Stone
!
@leesto
16
Everyone is invited to the review
1 x Senior must sign off
Reviewing Code Review
Lee Stone
!
@leesto
16
Everyone is invited to the review
1 x Senior must sign off
1 x Other must sign off
Reviewing Code Review
Lee Stone
!
@leesto
16
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
17
HipChat Bot
Because we all forget
Reviewing Code Review
Lee Stone
!
@leesto
18
Effective Code Reviews
Reviewing Code Review
Lee Stone
!
@leesto
19
Don’t Make it Personal.
Reviewing Code Review
Lee Stone
!
@leesto
19
Don’t Take it Personally.
Reviewing Code Review
Lee Stone
!
@leesto
20
I find using “we” rather than “I” or “You” helps
Reviewing Code Review
Lee Stone
!
@leesto
21
Don’t just point out the error or problem and move on
Provide Solutions
Reviewing Code Review
Lee Stone
!
@leesto
22
Put whitespace only changes into separate commits
Reviewing Code Review
Lee Stone
!
@leesto
23
Put ‘library’ code and ‘application’ code into
separate reviews
Reviewing Code Review
Lee Stone
!
@leesto
24
What Has Gone Well For Us?
Reviewing Code Review
Lee Stone
!
@leesto
25
Increased knowledge of similar work at sprint
planning sessions.
Reviewing Code Review
Lee Stone
!
@leesto
26
Tended towards standard approaches
in a quicker fashion.
Reviewing Code Review
Lee Stone
!
@leesto
27
Seen opportunities to refactor and produce
more generic libraries earlier.
Reviewing Code Review
Lee Stone
!
@leesto
28
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
29
Improved In-line Documentation
Reviewing Code Review
Lee Stone
!
@leesto
30
Increased Confidence From The Rest of The Business
Reviewing Code Review
Lee Stone
!
@leesto
31
Given Individual Developers More Freedom
Reviewing Code Review
Lee Stone
!
@leesto
32
Where Do I Have To Encourage The Team?
Reviewing Code Review
Lee Stone
!
@leesto
33
What are the most difficult reviews to
get feedback on?
Reviewing Code Review
Lee Stone
!
@leesto
34
Anything by
This Guy
Reviewing Code Review
Lee Stone
!
@leesto
35
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
35
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
35
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
35
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
35
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
36
Move Beyond Coding Standards
Reviewing Code Review
Lee Stone
!
@leesto
36
Move Beyond Coding Standards
Reviewing Code Review
Lee Stone
!
@leesto
37
What Do We Need To Improve?
Reviewing Code Review
Lee Stone
!
@leesto
38
Introduce Code Coverage Reports
Reviewing Code Review
Lee Stone
!
@leesto
39
Get Reviews Completed Quicker
Reviewing Code Review
Lee Stone
!
@leesto
40
Reviewing Code Review
Lee Stone
!
@leesto
40
Reviewing Code Review
Lee Stone
!
@leesto
41
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
41
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
41
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
41
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
42
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
Reviewing Code Review
Lee Stone
!
@leesto
44
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
44
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
44
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
44
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 Yo u r W o r k fl o w ?
Reviewing Code Review
Lee Stone
!
@leesto
44
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 Yo u r W o r k fl o w ? &
Think About…. How Do You Not Considerably Slow Your Release Pace?
Reviewing Code Review
Lee Stone
!
@leesto
45
Thank You.
Any Questions?
We know that code reviews are considered a good thing and 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 and what he'd like to improve this year.
for free. You
can too.