Code Reviews and How To Improve Them - Drupalcamp Bristol 2017

Transcript

1. Reviewing Code Review Lee Stone @leesto 1 Code Reviews and How To Improve Them. Improving the process of improving
2. Reviewing Code Review Lee Stone @leesto 2 Reviewing Code Review. Improving the process of improving
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
4. Reviewing Code Review Lee Stone @leesto 4 What is Code Review?
5. Reviewing Code Review Lee Stone @leesto 5 The activity of checking another developer’s code for defects and improvement opportunities
6. Reviewing Code Review Lee Stone @leesto 6 Checking code through a dedicated tool prior to merge upstream
7. Reviewing Code Review Lee Stone @leesto 7 Why Should We Review Code?
8. Reviewing Code Review Lee Stone @leesto 8 Prevent Bugs Reaching Production. Security Business Logic Coding Standards
9. Reviewing Code Review Lee Stone @leesto 9 Knowledge Share Ideal to mentor new developers Shared business logic - Remove SPOFs Discover Technical Approaches
10. Reviewing Code Review Lee Stone @leesto 10 Other Benefits. Improve Estimates Making Sure Solutions Meet Requirements
11. Reviewing Code Review Lee Stone @leesto 11 What Code Review Isn’t
12. Reviewing Code Review Lee Stone @leesto 12 It does not replace training
13. Reviewing Code Review Lee Stone @leesto 13 It does not replace demos
14. Reviewing Code Review Lee Stone @leesto 14 My Environment
15. Reviewing Code Review Lee Stone @leesto 15 Service Delivery Web Dev VoIP team CEO, Office Manager/HR Meeting Room 3 Meeting Room 2 Finance Spare desks Info Assur PG Finance Info Assur CEO
16. Reviewing Code Review Lee Stone @leesto 15 Service Delivery Web Dev VoIP team CEO, Office Manager/HR Meeting Room 3 Meeting Room 2 Finance Spare desks Info Assur PG Finance Info Assur CEO
17. Reviewing Code Review Lee Stone @leesto 16
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
19. Reviewing Code Review Lee Stone @leesto 17 Our Tech Stack PHP Zend / Slim Frameworks Split Between Libraries / Applications Mercurial, not Git
20. Reviewing Code Review Lee Stone @leesto 18 How We Complete Code Reviews
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
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
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
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
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
26. Reviewing Code Review Lee Stone @leesto 20 Everything is Reviewed.
27. Reviewing Code Review Lee Stone @leesto 21 Everything Should be Reviewed Within 2 Days
28. Reviewing Code Review Lee Stone @leesto 22 Everyone is invited to the review
29. Reviewing Code Review Lee Stone @leesto 22 Everyone is invited to the review 1 x Senior must sign off
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
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
32. Reviewing Code Review Lee Stone @leesto 23 HipChat Bot Because we all forget
33. Reviewing Code Review Lee Stone @leesto 24 Effective Code Reviews
34. Reviewing Code Review Lee Stone @leesto 25 Don’t Make it Personal.
35. Reviewing Code Review Lee Stone @leesto 25 Don’t Take it Personally.
36. Reviewing Code Review Lee Stone @leesto 26 I find using “we” rather than “I” or “You” helps
37. Reviewing Code Review Lee Stone @leesto 27 Don’t just point out the error or problem and move on Provide Solutions
38. Reviewing Code Review Lee Stone @leesto 28 Put whitespace only changes into separate commits
39. Reviewing Code Review Lee Stone @leesto 29 Put ‘library’ code and ‘application’ code into separate reviews
40. Reviewing Code Review Lee Stone @leesto 30 What Has Gone Well For Us?
41. Reviewing Code Review Lee Stone @leesto 31 Increased knowledge of similar work at sprint planning sessions.
42. Reviewing Code Review Lee Stone @leesto 32 Tended towards standard approaches in a quicker fashion.
43. Reviewing Code Review Lee Stone @leesto 33 Seen opportunities to refactor and produce more generic libraries earlier.
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
45. Reviewing Code Review Lee Stone @leesto 35 Improved In-line Documentation
46. Reviewing Code Review Lee Stone @leesto 36 The Business is Completely on-board
47. Reviewing Code Review Lee Stone @leesto 37 Increased Confidence From the Rest of the Business
48. Reviewing Code Review Lee Stone @leesto 38 Given Individual Developers More Freedom
49. Reviewing Code Review Lee Stone @leesto 39 Where Do I Have To Encourage The Team?
50. Reviewing Code Review Lee Stone @leesto 40 What are the most difficult reviews to get feedback on?
51. Reviewing Code Review Lee Stone @leesto 41 Anything by This Guy
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.
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.
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.
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.
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.
57. Reviewing Code Review Lee Stone @leesto 43 Move Beyond Coding Standards
58. Reviewing Code Review Lee Stone @leesto 43 Move Beyond Coding Standards
59. Reviewing Code Review Lee Stone @leesto 44 Don’t stick to giving the same feedback Especially on a ‘minor benefit’ point
60. Reviewing Code Review Lee Stone @leesto 45 What Do We Need To Improve?
61. Reviewing Code Review Lee Stone @leesto 46 Introduce Code Coverage Reports
62. Reviewing Code Review Lee Stone @leesto 47 Get Reviews Completed Quicker
63. Reviewing Code Review Lee Stone @leesto 48
64. Reviewing Code Review Lee Stone @leesto 48
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
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
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
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
69. Reviewing Code Review Lee Stone @leesto 50 These Delays Hurt Our Release Pace
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
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
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
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
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?
75. Reviewing Code Review Lee Stone @leesto 53 More Business Logic Feedback
76. Reviewing Code Review Lee Stone @leesto 54 Lets Recap
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…
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?
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?
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?
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?
82. Reviewing Code Review Lee Stone @leesto 56 Thank You. Any Questions?
83. Reviewing Code Review Lee Stone @leesto 57 https://joind.in/talk/e62d8