Effective Code Reviews: Case Study

As mentioned in previous posts, mentoring is an important and rewarding aspect to software leadership. One of the more effective ways to teach is via thought-out code review feedback. Performing code reviews is also an excellent way for the reviewer to learn and pick up new tricks. I often find it helpful to be on each end of the code review.

I solicit code reviews of my work when coding something particularly mission-critical and/or complex and encourage the same from other members of the team. There are many ways to run code reviews and perhaps I will discuss this in a future topic. Code reviews can involve a team or be one-on-one. The case study discussed here was a one-on-one case where I was the reviewer. Usually this has worked well for me. This situation had some unique challenges, however. Specifics of this situation:

• Working with a senior, talented, open-minded developer
• Developer is a non-native English speaker
• Developer is very open to feedback
• Developer's style resulted in overly complex code
• Developer frequently requested reviews
• Most of the request reviews span numerous modules

Problems this posed for me:

• Often times the code was just too complex for me to perform an effective review and root out potential side effects
• The language barrier reflected itself in the code in the naming of methods and variables
• The time demands of the frequent requests were too high

To address the time constraints, I applied a common technique, which was to ask to limit the scope to areas of highest concern. This did not work, as the developer was not able to identify specific areas. The inability to identify "chunks" that could be easily identified to treat in isolation was another symptom of the complexity.

This situation continued over a period of weeks. The bulk of my feedback centered on addressing the complexity issues and was in the form of email. However, each time I did this I felt I did not grasp the nuances of the changes well enough to provide adequate feedback. I found that the developer would make the specific requested changes and then the next day make the same complexity mistakes in the next piece of code that he submitted. To make matters worse, we were in a critical part of the project where time pressures were significant.

The real fix is to fully refactor the functionality. This is something we scheduled, however, it was not realistic in the short-term, so it was important to stabilize the code as much as possible. Equally important was my desire to help this talented developer with his complexity issues so that the problems would not be repeated in future projects.

Finally I decided that, despite the time-zone challenges and the additional time demands, the most effective way to perform reviews in this situation was to do them interactively with the developer. While voice is normally my first choice, in this situation I found that interactive IM sessions were more effective. There are two reasons for this:

1. It gave both parties an opportunity to think carefully before each reply
• From the developer's side this extra time gave him the opportunity to translate as necessary. 
• From my side it gave me time to analyze and provide recommendations as we went through the code.
2. It provided a written transcription for reference during and after the discussion. 

My biggest hurdles were understanding the motivations and side effects of each of the changes. So the review then became something like this:

• Me: Method X, lines Y-Z: What are you doing here and why?
• Dev: Explanation
• Me: Did you consider factors A and B?
• Dev: Actually, I think there may be a bug C 

( One of the things I like best about code reviews, whether I'm reviewing or being reviewed is that in discussing the code often the coder himself will stumble on issues)

• Me: What if you took this 4-line return statement and simplified it to a method call

etc.

The end result was that I felt I was finally able to provide effective feedback and identify specifics of how to reduce the complexity of the code.

The lesson learned here for me is to not wait so long to consider alternate communication mechanisms if I find one mechanism is not working well. This is a lesson I try to apply to many other aspects of communications.