Code reviews using negotiation tools
Part of the Agile Mindset series
This is the second of a series of articles exploring the current software development practices, including Agile, from the perspective of our cognitive capabilities.
The debate between doing code reviews vs. not doing code review is as undecided as it is vi vs. emacs. It is not my intention to try to settle that argument to any level, because I think that teams can do great with or without them, and it only depends on the culture.
Teams do code reviews in order to boost the conversation on implementation details. These discussions are important in order to not create knowledge silos, promote team collaboration and because we are delivering a description of a business process and that requires feedback. In my opinion, not having this conversation is not an option.
Clearly, peer programming is an alternative that satisfy above requirements. Peer programming sessions tends to be more dynamic with live interaction between the developers, and it expedites the integration of the changes. However, they can be energy draining for the involved people, as it requires the undivided attention of the people involved for a long period of time. Also, most teams lack the technology or skills for doing proper peer programming sessions when they are distributed.
As code reviews are time consuming, we should be wise on how we spend our time reviewing code (engineering time is expensive). The purpose of code review should not be:
- Correct style issues: lint tools can be used for enforcing style instead. All modern languages provide tools for style checking. Also, the IDEs have extensive style support.
- Find bugs: triaging a bug implies tracking the state of the code for different inputs. It is rather unlikely that we can efficiently do that tracking in our minds of code that has not been deployed while reviewing it. It is better to rely on extensive testing for finding bugs than in code reviews.
However if you decide to, there are two cases where I think that code reviews are extremely ineffective: very short reviews, and very long reviews.
When I am faced with a very short review, usually a change in the value of some parameter or flag, I usually do not have enough context for understanding the implications of the change. The only thing that I might check is if you used blank spaces around the equal sign in a consistent manner. And if I do have the context, it is likely that I am the author or I am profoundly involved in the task, so the code review from my point of view is unnecessary.
The case of the very long code reviews is more common. I can’t possibly infer the context and nuances of everything in a 10,000 lines code review. If it took several days to pull together the code, is not real to expect people to review it in one hour. I would argue that in these circumstances, it is more valuable to invite some folks to a meeting and have a conversation about the code. Keep in mind: the purpose is for people to understand your thought process, so it is cool if you can add a couple of slides with diagrams that allow people to catch the idea.
Having said that, I believe the code review is valuable as a communication tool that invites the discussion about the code. As any human interaction, it can be very valuable if it is done properly, but it can derail to an endless and meaningless exchange of comments (just like Twitter).
I couple of months ago, I finished reading Never Split The Difference by Chris Voss. This is a book about negotiating techniques, which on its core might not be applicable for a code review; however, negotiating is also a communication process, so I think that the same techniques can be applied on code reviews, but chasing a different end-goal. In a negotiation, the parties are trying to reach the best outcome for each of them; in a code review (or any internal team interaction, for that matter), we are trying to collaborate to reach the best outcome for the team, including present and future developers.
In this post, I will summarize some of the lessons I have learned from Never Split the Difference that are applicable to code reviews.
Imperative language vs Declarative language
Voss appeals through the whole book for the use of what he calls Tactical Empathy. The most effective technique for communication is to make sure that the parts feel listened. Nothing screams more: “I hear you” than me being able to repeat what the other party is trying to communicate. But is not just the words what matters, but the intention and the feelings that the message deliver.
The way we deliver our ideas plays a large role on that. If our language point fingers, blames, if we try to force our view upon our counterpart, and say people how to do things instead of questioning why are they doing things in a certain way, the communication won’t be effective and it will lead to unnecessary conflict.
On the other hand, if we arrive with real curiosity, trying to understand what the developer is trying to state and not making assumptions, but instead signaling what we are getting from the exchange, we foster the understanding between the parts.
In my head, this somewhat matches the paradigms of imperative language and declarative language. There might be better ways for labeling this language patterns, but using these allows me to not having to learn yet another paradigm, but reuse the one that we are familiar with regarding programming languages. For the ongoing discussion here, I will use this loose definition.
Mirroring
Mirroring techniques are used to ease the communication and build rapport and trust. They are more applicable for vocal communication than written communication, so they don’t map very well to code reviews. However, in order to effectively mirror your counterpart, you need to focus on what they have to say, and not on your own thoughts or preconceived ideas about the situation.
As developers, we tend to be very opinionated about the way code should be written. It is true that it feels great to say to someone that is wrong and tell them how to do things, but in the long term that doesn’t help the team as a whole. Instead, it is healthier to arrive to a code review with curiosity, trying to understand what the developer really wants to communicate.
Labeling
Labeling is a technique build on top of empathy, in order to bring to the surface unease feelings and dissipate them. Talking about things that makes us uncomfortable reduces the psychological burden it posses upon us. The key is to identify the feeling and then label it out loud it to the other part.
We can leverage from this technique the way that it exposes intent. If you’re uncertain of the intent of a piece of code or algorithm, you can use the language in the labeling technique for requesting clarifications. In order to be effective, frame your inquire with a neutral statement, and for additional points, end with an open ended question. It is important to use open ended questions because they make no assumptions on the idea and they don’t feel manipulative.
It seems like this function is trying to order a collection. What is the advantage of doing it like this instead of using a standard implementation from the collections library?
From time to time, people might get defensive about their code. The beauty of not using imperative language is that we can take a step back in that scenario: “I’m not telling that you’re doing that, it is just what it seems like to me”.
Leverage the No
The mastering of “No” is my favorite technique in the whole book. It is amazing how effective it is in both verbal and written communication. I think it tops as a tool for call for action, and it is great for closing emails that require an answer.
The idea behind mastering a “No” is simple: as an answer, saying “No” is a default defensive stand against changes. As we perceive changes as threats, saying “No” defend us against those changes. As such, “No” is an easy default question, and the more likely to receive when there is no clear tendency on one or other direction.
Whenever you need an approval for moving forward, frame the question in a way that a “No” will benefit your work.
Do you foresee any problems if we proceed with this project?
Will impact the budget if we use some additional virtual machines?
Do you perceive any risks on dropping this requirement?
Do we need to adjust our priorities further?
Is it too disruptive to move this snippet into its own function?
Would be a bad idea using functional programming instead of objects?
Is it difficult to add a test case for this scenario?
Subtle epiphany
There are situations where you want your view of the world to be considered. You can try to make a case from your point of view, and hope that the other part finally gets into your shoes and see the things from your perspective. However, it is more effective to help them discover the things that lead you to reach your particular view of the world.
In the negotiation sphere, this is achieved by making the other part say “That’s right”. Not “you’re right”, but “That’s right”. This is an acknowledgement that the perspective under discussion does indeed match the reality. I is an amazing way to settle interpretations as ground truth.
In order to trigger a subtle epiphany, we need to summarize first the context from the point of view of our counterpart. That reassures that we do understand the presented solution, and provides an space to check our own assumptions for building our view of the world. Then, expose the differences with a question — preferably an open ended question. Remember to use declarative language at all times.
It seems like you’re constructing the request object, adding the authorization headers, and sending the request asynchronously. How do we handle communication problems with the receiving service?
Never split the difference
Compromising, or splitting the difference, is a futile exercise. Neither part is satisfied with the outcome, so it is better to reach an agreement where only one part will be obtain what they need. This is not easy, as we tend to reach for compromise, as Voss states:
We don’t compromise because it’s right; we compromise because it is easy and it saves face. We compromise in order to say that at least we got half the pie. Distilled to its essence, we compromise to be safe.
In the case of code reviews, not compromising does not mean that the purpose of the reviewer is to make all of its comments to be addressed. Instead, we want to be sure that any proposal on the code, is not met half-way for the sake of avoiding the conflict. A half-baked solution is very likely to be difficult to read and understand, and most likely won’t deliver any message at all to future developers.
Avoid showdown
Directly confronting someone requires a lot of energy. Very large exchanges between parties, even when they sound exciting and show an image of progress, are highly inefficient. Usually, when you can’t settle on something with some few messages, it might mean that we require more information, or that we’re clashing egos.
That is not necessary. On software development, from my point of view, every hypothesis can be put under test by a unit test or something similar. So instead of confronting something that we suspect lies outside of the requirement, we can suggest a test to it. The proper declarative language on this scenario is to use open ended questions:
How does this function behave if we pass a null pointer to it?
What will happen to the resource if we raise an exception?
How does the code handle a race condition on the resource acquisition?
Avoid questions with “Why”. Even when it is an open ended question, it feels like an accusation. This is the only open ended question that belongs to the imperative language realm.
I hope these techniques allows you to leverage the potential of code reviews as a tool for communication and discussion of the details of the business in the implementation. I have one more thing to say about code reviews: we tend to skip tests while we review, as they are a mere formalism and not a communication tool as well. I would argue they are as important as the code itself. But that is something that I will review in another post.
