Some Thoughts on Code Reviews

Some Thoughts on Code Reviews

I lead an agile software development team. Reviewing pull requests (PRs) is a typical part of the software development process for us, just like many teams. Recently, my team decided to try an adjustment to the PR process. We configured the server so that two approvals for a PR were required and documented some team norms for how to put together a good PR. We also decided on a team norm for how long to allow the reviewers to review the PR.

As we were wrapping up our sprint recently, some concerns were brought up during our retrospective meeting. We had an excellent discussion and some key takeaways from that discussion seemed like a perfect blog post. Here are some observations and thoughts I had related to our PR process.

First, I prefer to view code reviews as opportunities to learn and teach. A development team can have any mix of senior, mid-level, and junior developers and the technology is always evolving. When a feature is being reviewed, this is a great time to discuss why it was done the way it was and to share alternate ways to solve the problem that might be better by being more efficient, more readable, less chatty, etc. For this to work, your team members need to feel psychologically safe and have a good rapport with each other. If the discussion is bigger than a few comments on a PR, consider setting up a meeting with the developers or the entire team to discuss what the best solution for the feature is and why. This leads to my next thought.

Be prepared to defend your implementation choices (in a professional manner). Ideally, if I ask a team member "Why did you decide to implement this feature this way?", they'll have some thoughts about it. It could be that thought it would be clearer for others to maintain. Or, they considered doing it another way, but it seemed like overkill for this particular feature. If the code is just copied and pasted from StackOverflow and the developer can't defend it, it could be another teachable moment.

When I started working as a Senior .NET Developer at an insurance company, the development teams weren't doing a formal code review process. My manager and I worked with our team to create a process for formal code reviews. One of the things we did was create two kinds of code review comments. Some comments were marked "required" and some were marked "optional". This allowed team members to make suggestions that may not improve the functionality of the code but might have some other benefits. This was before the organization had implemented agile scrum, backlogs, etc. In an agile environment, you could create Technical Excellence or Technical Debt tickets for such suggestions, as well. The point is that if your review process only focuses on deployment-blocking issues with code, there might be some discussion or improvement opportunities that get missed.

When it comes to discussing PRs, I strongly advise everyone involved to choose their words carefully. For example, I had one senior developer comment on a PR that a chuck of code "seemed lazy". The developer who wrote the code took that as being described as lazy. Writing software is an art and a craft and some people naturally get attached to what they built. Avoid being funny, insulting, and generally unhelpful with PR comments. A software development team should not allow the PR review process to damage team morale or degrade collaboration.

PR reviews come at the end of the development cycle, but problems earlier in the process can impact the efficiency of the PR review process. If the team notices an unusual amount of PR rejections, discuss the quality of the user stories or feature requests. If the requirements aren't clear or if they lack any technical guidance, rework is almost guaranteed. On my current team, I have a Solution Architect (SA). When a non-trivial work request comes in or we pick up a new project from the PMO, the SA will review the request and design a solution to meet the business need. My SA tries not to provide a solution that includes "write this class, use this pattern, use the database schema". He doesn't want to be prescriptive. However, when the PRs started coming in, he had a relatively high rejection rate. The developers weren't building it to his expectations. This frustrated the developers because the SA didn't say exactly what he wanted. Had more technical details been included in the stories, the developers would have built it that way. From the perspectives of my SA and my Staff Developer, if they have to spell out every bit of how to build it, they might as well have just coded it themselves. To solve this problem, I worked with the more senior developers to come up with key topics for training. We sent the team to formal training sessions for larger industry topics. For more organizational or platform-specific training, we set up training meetings as "lunch and learns" and I purchased lunch for everyone. This allowed the more senior team members to share concepts and design patterns so that the team could get to a common understanding of the best patterns and technologies relevant to the work the team was doing. Once all the developers were speaking the same language, the stories became clearer and the developers built solutions that met expectations more often.

My last thought is related to a software development style guide. For some teams, the formatting and the style of the code are important. I worked with a NodeJS/React team where some of the developers used semicolons and some didn't. It was mildly distracting to the team when they compared revisions of the code if semicolons were present or missing and if curly braces weren't in the same places. In my opinion, the PR review process should not spend any time on these kinds of preferences. There is no business value in it. The NodeJS team I worked with met as a team (several teams, actually) and decided to adopt the style guide that Airbnb used for their code with some minor tweaks. Then, they implemented a linter. The linter was able to automatically remove semicolons, format the code so the curly braces were consistent, and even detect issues that a developer needs to address prior to checking the code in. By implementing this process and tool, the PR review process was able to focus on the proposed solution and whether it solved the problem for the business in the right way.

My current team is still working to optimize their processes and improve their communication. If I have any additional insights, I'll be sure to share them. Until then, happy coding!