A Review a Day Keeps the Conflicts Away

I used to tweet a lot of articles. I mean A LOT! To the extent I am mocked as the team Tweetaholic. There are definitely a few gems in there. Nevertheless, one subsequently deleted find on code review size definitely stirred some active debate in the office.

 

Hence my first official collaboration with the amazing Joe C. Think of this as our rebuttal to why code reviews are less broken than they appear, especially if trunk-based development is adopted.

 

Moving the End Zone

 

We have all been there, especially us authors. You are asked to review a massive pull request touching a ton of files. The majority of these appear at the start or end of our day as opposed to just before lunch.

 

Large requests such as these stem from developers getting into the zone. Coding happily for hours on a massive task that gets progressively bigger. There is nothing more exhilarating than the magical feeling of your hands dancing over the keys. However, to reduce merge conflicts we need to exercise some collective control. We mandate developers not only pull recent changes into their local fork, but push at least once every 24 hours.

 

This constant collaboration ensures developers are not slowed down by each other. Furthermore, it encourages communication and discussion which expands their skills. Small and steady on this occasion definitely wins the race!

 

Owning the Collective

Another rather surprising observation that took us aback was the notion of a single reviewer for a single code base. Having an individual responsible for any task is doomed to fail. By assigning an exponentially high bus factor to any one person, you are running the risk of halting the code production line if they get sick, encounter hardship or burnout, or are hit by that mythical Knight Bus.  

Part of being an empathetic colleague is understanding when we all need to chip in. Understanding managers must be quick to identify members of their team that are struggling. This single point of failure invites disaster into the software house.

 

Under Pressure

 

The main pre-lunch pressure scenario is troubling for the authors. The reviewer in this case felt pressure to merge code despite being uncomfortable doing so. Did it really need to be done as urgently as the requestor suggested?

 

The whole premise behind a code review is to make sure developers and reviewers alike understand the consequences of a merge. Furthermore, we want to make sure that the quality of the code being committed is as high as possible, and adheres to the team’s collective standards and definition of done. For this to happen, it’s important to know who the right people are to review such merge requests, and to certify that those individuals are the ones to provide the final sign-off. Both authors have very different skill sets, coming from the opposing server and UI sides of the force. Nevertheless, we both regularly ask developers who have sent us reviews outside our respective areas of expertise to send reviews to the other. This ensures that the purpose of the review is met. OK, developers have sometimes ended up waiting a few extra hours before the code is merged, but it has also meant more meaningful feedback is provided to the reviewee. It is both the authors’ opinion that the targeted feedback provided is worth the additional wait time.

 

Think of a code review as a stationary bike. Setting the resistance as low as possible may give you a feeling of immediate satisfaction. Regardless, you will eventually find yourself wondering after a year why your code muscles are no stronger than before. By avoiding the path of least resistance, and putting up with feeling like your muscles are on fire, coding will become easier as your muscles begin to strengthen. Any good coach will tell you the best place to go to build such strength.

 

Absolute Power Corrupts Absolutely

 

In this scenario, a single reviewer has power over the entire team, and can dictate which shall pass. Any team allowing such a model are setting up that individual to be the most hated person in the team. Developers will see a dictator who stopped my code from being adopted. They stopped the dancing of my fingers across the keys. And they will bite back, hopefully not literally! Reviewers do tend to be more experienced, so managers need to consider if they can afford for this person to become miserable, unhappy, and ultimately leave as a result of the regular infighting that they will have to endure from others.

 

Having a single gatekeeper enforcing which code shall pass can lead to animosity among the team

Using this single gate keeper to dictate what code shall not pass could improve quality, if the reviewer is good. Nevertheless, the code will most likely confirm to their ideal architecture, their best practice and their code style. Unfortunately that means their bad habits will also creep in (and we both definitely have bad habits, as much as we don’t like to admit it)! Diversity of thought based on experience in life and code is vital to driving high standards and writing better code. Only by encouraging cross review can you strive to enforce collective ownership. We don’t agree that collective ownership is a fallacy. In this model you encourage single ownership. By distributing power and responsibility, you can encourage happy, productive developers, whose fingers will dance the night away over the keyboard.

 

Learning is More Than Doing

 

One aspect of code reviews that does often get lost is the learning aspect. People learn by more than just doing. One author recently highlighted that coding continually will not make you a better developer. Practice is certainly important, but how effective are we at reflecting on that code later? Could we, even as senior developers, benefit from the feedback of others on our own code? When can we reflect, and learn from a larger community pool without resorting to reading and ingesting the wisdom of others?

 

Being a code reviewer is an important role in the development of any junior engineer. You have an opportunity to instil best practice, and impart some of the advice you wish you had received a few years earlier in the exact same situation. Arguably it is even more crucial that reviewers take their role seriously and incorporate learning from the vast array of books, articles and other resources that our industry is fortunate to churn out regularly. Whether you like it or not, you are distinguishing yourself as a role model by reviewing their code. With great power, comes great responsibility.

 

Thanks for reading! Special thanks to Joe C for his contribution!