Talks
Code Review: Pitfalls and Good Practices
Summarized using AI

Code Review: Pitfalls and Good Practices

by Thong Kuah

The video, titled "Code Review: Pitfalls and Good Practices," presented by Thong Kuah at RubyConf AU 2016, explores the significance of effective code review practices in software development. The focus of the discussion is on the cultural aspects of code review rather than merely technical checklists, emphasizing how a positive environment can lead to better code quality and team satisfaction.

Key points discussed include:
- Importance of Code Review: Code reviews are crucial not only for identifying errors but also for nurturing a positive team culture and enhancing developer satisfaction.
- Cultural Focus: The speaker stresses the need to get the right culture in place before delving into technical practices, as a supportive environment leads to better outcomes.
- Initial Challenges: The team faced significant challenges in 2015 when their project suffered delays, compounded by large, risky code changes.
- Review Process Changes:
- Transitioning from end-of-task reviews to more frequent, task-based reviews to reduce bottlenecks.
- Encouraging team-wide participation in reviews, allowing junior members to contribute valuable insights.
- Mandatory Reviews: Implementing strict code review requirements improved overall code quality.
- Defining ‘Done’: Establishing clear criteria for what constitutes completion helped to reduce unnecessary feedback.
- Incremental Code Changes: Breaking down large projects into manageable chunks allowed for timely feedback and reduced stress during releases.
- Engagement with All Team Members: Involving the whole team in code reviews created a learning culture and brought varied perspectives, which helped catch mistakes often missed by more experienced developers.
- Collaborative Review Practices: The introduction of a collaborative model where team members review each other’s work encouraged comprehensive feedback and fostered relationships within the team.
- Role of Testers and Review Managers: Testers and a release manager were essential for quality assurance, providing additional layers of scrutiny before code deployment.

Ultimately, the team saw marked improvements in their sprint performance and happiness, which led to their practices being adopted by other teams within the organization. Thong encourages viewers to reflect on their own code review processes to enhance team productivity and satisfaction.

00:00:02.240 Good code review can be a fun, positive, and enriching experience. It helps you establish a clear review program that can significantly benefit your current developers and potentially attract new talent.
00:00:09.920 That's certainly what we experienced at Powershop.
00:00:24.080 Hello, I'm Tong. Here are some ways you can reach me after this talk. I'm currently working as a team leader at Powershop.
00:00:36.079 Powershop's mission is to help our customers better understand their power consumption. We achieve this by giving them better control over their power usage through various means, such as straightforward understanding of their meter readings and providing online bills in an easy and fast way.
00:01:01.920 To facilitate all this, we have a single monolithic Rails application that powers almost all areas of our business. This includes our online shop, the call center, and handling file exchanges with other parties in the industry.
00:01:36.240 During this talk, I will discuss how my team improved our current review process. I want to emphasize that this talk will focus more on the cultural aspects, as I believe that if we get people and practices (also referred to as culture) right, then the technical aspects will naturally follow.
00:01:44.400 There are plenty of blog posts and videos about technical checklists for good code reviews, but the cultural side often receives less attention. Therefore, I want to highlight that today.
00:02:10.319 More importantly, it's about transitioning from a state of panic, exemplified by feeling overwhelmed, to a place where we have a happy team that can efficiently ship productive code.
00:02:31.840 Around April 2015, our team faced significant challenges. We were in the middle of a large project, and in the last nine sprints, we failed to complete our tasks on time. This was quite disheartening as it left us feeling deflated.
00:02:41.760 We struggled with a series of extensive changes, essentially refactoring half the application. This involved engaging in multiple review cycles, some with external reviewers, and we received substantial feedback along with extensive review notes, which we found challenging to manage.
00:03:27.200 These large changes required intricate code adjustments that were risky. Initially, we spent excessive time reviewing, especially as the stakes were so high. During that period, the average size for a code change was around 1,000 to 2,000 lines of code.
00:03:54.639 Getting a 2,000-line code review at the end of the sprint, with only a day to address it, was particularly stressful. This was compounded by external testing pressures. We were not completing our sprints on time, and this slow pace meant that our team was only able to release new code to production about once a month.
00:04:34.320 Releasing code to production that involved large and risky changes carried inherent risks, as encountering issues was nearly inevitable. We discovered this during one of our production deployments, which thankfully did not result in data loss.
00:05:18.240 In the retrospective for sprint 119, our team resolved not to endure such chaotic processes again. We decided to adjust how and when we reviewed code, opting to conduct reviews more frequently— not just at the end of tasks.
00:05:36.640 This entailed breaking changes down into smaller increments to make them more manageable and minimizing the necessity for massive changes that might prove unnecessary.
00:06:01.680 Another change was encouraging everyone to participate in the review process. We’ll delve deeper into this shortly.
00:06:57.919 Before I outline what we changed in our code review process, I want to highlight what we were already doing well. First, we had made code reviews mandatory, which is not the case in every organization.
00:07:22.720 Having mandatory code reviews has proven invaluable. This practice has helped catch numerous issues that might otherwise go unnoticed across our seven years of conducting reviews at Powershop.
00:07:40.560 Secondly, we established a definition of done; this is crucial because it prevents unnecessary suggestions and avoids prolonged feedback on trivial matters. If a review highlights a minor improvement and it doesn’t align with our guidelines, we can defer it.
00:08:12.320 Thirdly, we emphasize testing our code. Having specifications in place is slightly more critical than the review process itself in terms of safeguarding us against bugs. The number of embarrassing issues we've caught through our testing has been impressive.
00:08:50.240 Thus, the first change we decided to implement was to increase the frequency of reviews. Previously, we only conducted reviews at the end of the coding process, which created a significant bottleneck.
00:09:02.720 To address this, we resolved to review code after completing every task. For instance, if a change comprised three tasks, such as writing the model, creating the service, and integrating everything into a controller, we would review at the end of each of those tasks.
00:09:54.800 We found this approach worked well, as it allowed for timely feedback. If there were issues, we could address them immediately instead of waiting until a larger review at the sprint's conclusion.
00:10:06.800 Initially, we attempted to review after every commit, but this proved overwhelming due to the number of commits. Eventually, we returned to task-based reviews, which yielded better results.
00:10:30.839 Another significant change was ensuring that everyone in the team remained aware of each other's tasks. Before this, we often divided into smaller groups working on separate changes, only to reconvene for reviews two weeks later, which wasted time.
00:11:12.720 To optimize this, during the early stages of a sprint, we would hold a planning session to outline the high-level tasks. This small adjustment allowed team members to familiarize themselves with changes, improving the speed of our reviews.
00:12:01.199 To recap, we now plan out work collectively, encourage frequent feedback after every task, and maintain a final review before merging into the master branch. However, frequent reviews alone didn’t fully resolve our issues regarding large code reviews.
00:12:32.080 We subsequently engaged with our product owner to break down changes into smaller, more manageable independent chunks. This process benefited everyone: the product owner could see incremental progress, and testers had more code to work with instead of waiting indefinitely for us to complete everything.
00:13:00.000 Additionally, we recognized that a limited group touched the code reviews, usually involving me and one or two senior developers. Through discussions, we decided that everyone should take part in the reviewing process. This fostered an immersive learning environment, particularly for new team members.
00:13:45.680 Explaining the mechanics of reviewing and offering them opportunities to engage, even if they were new, proved beneficial. I’ve seen junior members give incredible feedback and catch mistakes that even the more seasoned developers missed.
00:14:09.840 For instance, I had a junior reviewer identify numerous bugs in my code, which was impressive. Such experiences demonstrate that even newer developers bring valuable perspectives to the reviewing process.
00:14:31.200 Sure, minor bugs slipped through occasionally, but the trade-off was worth it. A quicker feedback cycle allowed new developers to learn rapidly, while we still managed to catch major issues before production.
00:15:14.000 Additionally, we retained a final review point before merging to master. In the next team I joined, we adopted a collaborative review model where each member would review each other's code, resulting in more comprehensive feedback.
00:16:03.680 Moreover, reviewers have the option to seek second opinions from outside team members, which we affectionately termed 'second breakfast'— a nod to The Hobbit. This practice is particularly useful when dealing with complex code involving finance or data integrity.
00:16:32.320 Furthermore, our team of testers plays an essential role in quality assurance, helping validate code changes. The release manager also conducts quick reviews to ensure nothing might disrupt production.
00:17:11.040 Six months later, the changes we made resulted in significant improvements in our sprint performance. The team has been consistently happier as we achieve satisfactory outcomes time and again.
00:17:25.160 Other teams began adopting our processes, which has been flattering as well as beneficial to their own projects. Powershop has also seen tremendous growth, expanding from 30 to 50 developers and evolving from three to seven teams.
00:17:46.640 To summarize, effective code reviews can provide an amazing experience for both current and future team members. I encourage all of you to reflect on ways to improve your teams to enhance their workflow and, ultimately, foster a more productive and happy environment.
00:18:06.370 Thank you.
Explore all talks recorded at RubyConf AU 2016
+11