On code reviews

Code reviews and collaboration

Here’s one interesting topic to think about; Code reviews and collaboration. One small step towards the bigger dream. Sounds easy right? Yet a lot of issues seem to occur now and then that makes us stumble and struggle trying to find the sweet spot on how to treat code reviews while others treat them as of little importance (let’s not discuss about teams that don’t have code reviews) only to come up with teamwork issues in the future without realizing that wrong handling of the code review process is one important factor for their teamwork demise.

Code reviews as a conversation starter

What are code reviews if not a conversation starter for a proposed solution to a problem? The author which is the starter of the discourse has come up with a solution to a specific problem (by “problem” I don’t necessarily mean a bug; a problem can be a new feature as well. A problem is anything that needs something to be done about it). They think their solution is right and sometimes they don’t have a solution yet but they’re working on a solution (usually marked as “WIP” for Work In Progress) that they think is right. So why do they need a code review? Are they losers that don’t know how to code solutions and need someone to hold hands with? Are they afraid to deploy their code?

None of the above. Code reviews act as a teamwork booster and at the same time have a ritualistic purpose. The solution the author has come up with, is transparent and set to the tribe for judgment, understanding and discussion. The team will read the code, try to understand it and then discuss about it to answer the following questions:

1. What problem(s) does this solution solve?
2. Is the problem understandable by everyone in the team?
3. Is the solution working or are there edge cases that need to be taken into account (and either rethink about the solution or dismiss the edge cases as non-applicable)?
4. Is the solution understandable by everyone in the team?
5. Is the solution in accordance with the methodology and implementation techniques the rest of the team uses?

Most -if not all- of the points above might seem obvious but I’ve learnt the hard way that in reality we tend to forget many of them in our everyday work. One important reason is that in most cases one person takes the role of the reviewer (although I’ve seen teams that pass around the reviewer role until most -if not all- of the team members have given green light) due to lack of time and resources. One other reason is that there’s a trend in the industry to “move fast” which is usually conceived as “if it works we’re okay no time for polishing”. See how the last sentence deals only (and partially) with one of the aforementioned points (#3) ?

So what?

“So what if I don’t care about the other points? Since this works and one reviewer (if any) thinks it’s okay we should deploy” is one classic argument (that is usually accompanied by the time factor argument). The problem that is created in such situations is that ignoring the rest of the crucial questions we reinforce a false image about the way the team is built.

1. What problem(s) does this solution solve?

   “But I use TDD! I’ve created tests that reproduce the issue so it’s fine!” some say. Unfortunately reproducing an issue doesn’t mean we know what the problem is. Yes we might have reproduced the context in which the problem occurs but this doesn’t mean we understand why the problem occurs. Lack of such knowledge leads to patching issues. Patching issues may seem fine for some time, but usually (if not always) ends up in a big pile of untangled mess where the only solution is to burn the code base and start from scratch. This usually is realized when we reach a tipping point where a seemingly simple issue fix triggers a lot of counter-effects that should not happen.

2. Is the problem understandable by everyone in the team?

   If we don’t know the answer to this question then how can we expect the team to have a good grasp of the way the product is built? Not having this knowledge shared on team level means that there are gurus in the team. A guru might seem cool but at the same time is the single point of failure. A person that if they ever leave everything will collapse and at the same time a person who is in constant risk of burn out. If time factor is of any value to your product you should pay even more attention to this. Otherwise, you’ll reach a point where everything will take more time because the “guru” will have to give their approval/opinion. This will mean that they will have to take care of everything which as a result will take more time and eventually burn out the “guru”. Let’s not even discuss what will happen if for some reason the “guru” needs to take a leave or wants to leave the product/team/company.

3. Is the solution working or are there edge cases that need to be taken into account (and either rethink about the solution or dismiss the edge cases as non-applicable)?

   How can we answer this question if we don’t know how the team thinks? In most cases there isn’t time for everyone to take a look on the solution so this decision is for the reviewer to make. The reviewer should answer this question as a team representative. That is, they should try to think the way the team would think about this solution and try to come up with possible issues.

4. Is the solution understandable by everyone in the team?

   This question might seem covered by point #3 but in reality it deserves an extra point. The burden the reviewer has is not only to think as a team about the solution but also to try to understand the solution as a team. This means they should ask themselves “Will X understand this solution? Will someone new to the team understand this solution easily?”. This question is important to answer since this is the pitfall where most of the time we end up seeing “black magic” solutions where in the future you might hear team members say “I don’t know why this works, but it works and we don’t touch it”.

5. Is the solution in accordance with the methodology and implementation techniques the rest of the team uses?

   Again this might seem to be covered by the previous point but it’s not the same question. This is one of the tricky questions needed to be answered. One other way to ask this is “If X implemented a solution to this knowing how to solve the problem, would they solve this in a similar way?”. Where “similar” means “in a similar manner”. If for example the team has decided to use object oriented programming for 99% of the project, solving things in pure functional programming style without having decided as a team to make such change of mindset is like adding truck tires to a Lamborghini. Yes it might work. Yes it might run faster (adding truck tires to a supercar will certainly not though) but a big step towards creating a Frankenstein has been made. If this was not a team decision it might mean that either the author will only touch this code from now on or that when (and if) everyone else will try to tamper with this code they will contribute to adding more pieces to the Frankenstein experiment.

Common pitfalls

Sounds easy right? How difficult can it be to pay attention to these five points? So why do we keep failing to perfect our code reviews? A lot can be said here and most of them might be obvious in most cases (lack of time, lack of interest from the reviewer, pressure from the project manager to ship fast etc) but there are also some times where the answer is not totally clear and things seem to move forward only to sink like the titanic in the future.

1. Bad / Wrong explanation of the problem

   It is the author’s responsibility to provide a clear explanation of the problem the solution is solving. The author has spent (hopefully) some time understanding the problem and it is their responsibility to pass 100% of that knowledge in a very condensed time-bit. The reviewer (and as a result the rest of the team) does not have the luxury to spend that much time searching about the problem. So they must read a clear explanation of the problem and understand its core as fast as possible. We usually create code review request saying very few things about the problem and how it occurred assuming that the reviewer would either already know this or that it is of little importance. But this information is very crucial and must be written down and referenced in the future. This is one of the core bits that help build knowledge about the product. Explaining a problem (in cases of new features this means explaining possible cases taken into account) also helps the author make sure they understand the problem as well. There’s a saying that goes “the only way to make sure you know something is to explain it to someone else”. Always make sure that the problem is clearly stated.

2. Bad / Wrong explanation of the solution

   Code speaks better than words right? Most of the times yes, but when it comes to reading a code review or product history we might not have the luxury of reading every code bit of the solution. Plus the code does not explain why a specific solution was preferred instead of another. One might come up with multiple solutions to a problem but many of them might have edge cases. That’s why we must make clear why this solution is the best. This is also a part of understanding the problem. Understanding why other solutions don’t work means that you understand even better what the problem is. This also helps in future cases where we might revisit a part of the code thinking that we could change it to also incorporate an extra solution to another problem. For example you might try to use a new Y solution to some part of the code that would also solve a newer problem. So you start saying “Okay instead of the old X solution I will use the Y solution” only to realize from the commit message that “Y solution was not used because it results in some fuzzy issues”. So now you know that Y is not an option and that you should come up with another solution. If that explanation was missing you would have solved one new problem only to bring back to life an older problem with a fuzzier look.

3. Lack of discourse

   This is a general category that can fit a lot of cases. Remember that this discourse is not verbal. This means there’s no body language, no eye contact, nothing. Just written sentences and at best emoticons. So one should write and read very carefully so that things are not misinterpreted. Toxic comments like “this is dumb” are obviously bad but sometimes even simpler comments that are more laconic might be treated as bad comments. There’s another saying that goes like “Treat your code reviews as if the other person is a psychopath murderer”. Instead of writing “This is not working” try writing “I’m not 100% sure how this is working, can you explain it to me”. The role of the reviewer is not only to judge if the code fits the team but also make sure the author fits the team. Instead of suggesting how things should be implemented try discussing it with the author and come up with a common suggestion. This is what builds team understanding and boosts teamwork. Creating a common projection about the product. This is not as easy as it seems since most times we tend to think we’re always right. This means that we’re judging opinions that do not fit ours and try to steer the other opinions to match ours. This is not discourse, this is persuasion. There’s even worse than that. Overriding the author’s work to match the reviewer’s thought of the solution. This might be in form of direct implementation suggestions (orders in disguise) on specific code parts or even worse reviewer commits in the author’s review request. I think the latter is the worse since it overrides any sense of discourse from a code review. I’m not talking about minor changes that would in 99.9% of cases be suggested by the reviewer and accepted by the author but to save time the reviewer makes them instead. I’m talking about changes the reviewer makes that change a solution the author spent time to suggest. What’s the point of the code review request in the first place? And how does the reviewer’s overriding answer to the 5 questions a review process must answer. I find such cases highly problematic since not only they are disrespectful to the original author but at the same time they act as a discouragement for future code reviews. Having situations like that will soon result in the author feeling dismayed and avoid asking for code reviews since they will stop feeling part of the “team brain” for the product. Scale this up for some time and you’ll end up having a team were most members will not understand how the product works and why it works like this and/or will play safe with simple commits. The product will soon reach a phase where the only accelerating factor is the “whiz kid’s” passion to get involved into everything. Good job, you stopped having a team. And don’t worry, there’s even worse than that! Passively overriding the author’s work to match the reviewer’s thought of the solution. This usually happens in toxic places where there’s so much passive pressure or toxic environment where the author does not even want to start a discourse with the reviewer (because it won’t be a real discourse). So the author is trying to satisfy the reviewer’s point of view and “get over with it”. I think this is even worse than the active imposing of the reviewer’s point of view mainly because now we have the same problem but the problem stays well hidden. So everyone acts like everything’s fine when in reality everything sucks. “And what about team”, you ask? There’s no team, only minions!

4. Lack of reviewers

   There are some cases were we see that every code review is destined for one person. It may not be an official BDFL but unofficially they act like one. The problem with this should be obvious by now. Team knowledge is not built if only one person gets to judge what is inside the “team’s brain”. Try passing around the reviewer role. If the second reviewer came up with something the first one didn’t notice, try asking (first yourself) why this happened, if this is okay (and somewhat expected maybe?) and how will this not happen again (hopefully not by choosing only one person to do the code reviews!).

Closure

That’s all for now! I’m sure I’ve missed mentioning a lot of stuff. Hopefully I’ll remember some of them and add them. If you had to keep only one point from the above, please keep this: Code reviews are an integral part of the team building process.

My thoughts on this are not over. There’s still one elephant in the room that needs to be addressed and is called “community open source projects” where terms like “code reviews” “BDFL” and “community” are used in the same sentence and some times are contradictory to one another. But I’ll leave this for some other time…