Code Review – Why Nobody Wants to Read your Code?

Well, can you blame them?

Code reviews might be the only thing in software engineering that is universally considered “good” and “the right thing to do”. Furthermore, it is one of the few things that are empirically proven to improve our software [1].

And yet, I oftentimes hear particular about reviews. Nobody complains about the concept, but a lot of people (I am tempted to say “everybody”) are not 100% satisfied with the implementation.

The most common compain I have heard is about the lack of timely reviews.

In this article, I will try to answer the question in the title – why some diffs are neglected while others get more attention. And more importantly – what can the author do to make his code a “bestseller”.

Note that I will be using diff and pull request interchangebly throughout the article. Also, I assume there is already a code review practice in your team. If there isn’t – this article will not be particularly useful and you must first establish code reviews before making them better.

Top 3 reasons why people avoid reviews

My insights suggest that there are 3 top reasons why people tend to avoid reviewing particular diffs:

  1. Volume. Not surprisingly, since large code changes require lots of time and focus to review thoroughly (sometimes more than it took to write them). Many engineers use time between meetings, morning coffee or some other convinient slot to do reviews.
  2. Complexity. Very similarly to volume, code with high complexity requires time and mental energy to review properly. It is of high density – a small piece of complex distributed problem solution has the same “mass” as a much larger mundane diff.
  3. Lack of context. If you are not familiar with a piece of code and the reason behind the changes, you cannot actually review it. Living linters, although not uncommon, don’t bring much value to the table – the purpose of the code review is to verify the correctness of the proposed change above anything else.
  4. Value. It wouldn’t be a top 3 without a number 4, would it? Engineers are particularly lazy (in a good sense). If your diff doesn’t bring  value or it is not clear why it is important, people will tend to neglect it.

How to make your pull requests better?

The heart of my advice is simple – prepare your diffs for review with empathy.

Empathetic writing is a well-established paradigm, one where you constantly try to be in your reader’s shoes. You can start exploring it on your own – there are tons of materials (e.g. https://www.grammarly.com/blog/empathy-writing).

Start by asking yourself:

  • Who is going to review my diff?
  • What is her background?
  • What context does she have for the given task/problem/code?
  • What is the best/easiest way to read through the changes?

Every communications training urges you to think about the audience. While some may argue that you write code for the compiler (I strongly disagree with this statement), nobody can deny that the intended audience of a pull request are software engineers.

Think of ways you can help or guide your reviewers. Here are some things I like to do:

  • Discuss a change in advance whenever possible. Before you have written a single line of code the level of ego is at its lowest and you can really benefit from a fresh perspective. This also gives the other engineer(s) a sense of co-authorship which is priceless during the review process.
  • Review your own code before giving it to somebody else. If you are not already doing this, you will be surprised how effective a self-review might be.
  • Start with why (Simon Sinek reference intended) – explain why your diff is important. Don’t forget to be empathetic with your reader – who is she, would your explanation make sense for someone without your context, how is the change important to her, etc.
  • Write a short summary, explaining the entire diff so that the reviewer can read your code while keeping the general idea in her head. I sometimes use this to guide the reviewer to read the files in a particular order that makes most sense. Avoid “retelling” your code (e.g. explain why you are committing the transaction at line 356 rather than stating you are doing it).
  • Give enough context of the problem and code/components under review. Try not to give too much. You can put the ticket (e.g. Jira), but, please, don’t stop there – I have rarely seen tickets that are good enough as just references. Extract what is important from the ticket and put it in the pull request description.
  • Give references to documentation, especially if understanding the change requires deep knowledge about some library/third-party component. This has the added benefit of making you look at what the documentation is actually saying and verify some of your assumptions.
  • Don’t be clever. A subject of its own article, clever is not the same as smart and being clever in your code is rarely good for your reader.

As you can see, all things revolve around thinking about your reviewer saving her precious time. Also, in order to make the best out of your reviews, you must start thinking about them early.

Conclusion

I hope you don’t treat this article as a checklist, but rather as an inspiration – ask yourself questions, be empathetic with the readers of your code and pull requests, think of ways you can help them help you.

References

  1. The impact of code review coverage and code review participation on software quality: a case study of the qt, VTK, and ITK projects by McIntosh, Kamei, Adams, Hassan
  2. Peer Reviews in Software: A Practical Guide by Karl Wiegers
  3. Code Review at Cisco Systems by SmartBear
  4. The CL author’s guide to getting through code review by Google

 

Photo by Engin Akyurt from Pexels

Code Reuse – the Good, the Bad and the Ugly

Recently, I had a very intriguing conversation around reusability, team independence and self-contained services. Since I’ve been reading about this in the context of microservices and modern immutable infrastructure, figured out it is an interesting topic for a post.

There are a number of practices and technologies claiming to solve the problem with dependencies by preferring self-contained services or no-code-sharing policy. Although this is a useful concept, we (software engineers) sometimes underestimate the complexity of composite systems regardless if they are monolithic or service-oriented. We should analyze the dependencies in our system carefully, track them and monitor them instead of trying to ignore and hide their very existence.  Service-reuse is a powerful tool, but it is transforming the problem of handling dependencies and not solving it.

First of all, why do we have dependencies in our code? Following the logic of Conway’s law, the challenges of managing dependencies are closely related to the broader concept of reuse and are a reflection of the people developing the software. If you are not a lone wolf working on some small (pet) project, you have to deal with dependencies, regardless if you are aware of that or no and no fancy hype like containers or serverless can save you from that; you have to sit down and engineer you way around this.

Classic Dependency Management

The standard form of reusing code is through dependency management tooling. We describe the libraries and components we are using with their versions in some declarative form and use the corresponding tool (gem, npm, Maven, nuget, CMake) to resolve them and bring the artifacts locally. After that we link them to our program either at compile time (statically) or at runtime (dynamically) and we are good to go.

As software is becoming more complex and more libraries are added, we eventually face the dependency-hell problem. Different components of our solution require different version of the same library while in a single runtime you can have only one. And there are even more challenges: we need to update those dependencies regularly, we scan them for vulnerabilities, exploits and bugs.

Self-Contained Microservices

Microservices to the rescue! If we split our software into hundreds of small self-contained programs, each running with its own view of the shared libraries and filesystem, we no longer have the problem of dependencies, right?

Let’s take a look on a higher level. Those services communicate with some form of RPC, so how is the mechanism implemented? In every service? What if we need internal trust mechanism? Then we can have some common code (note we are not tackling the challenges of polyglot system) to handle this. But what happens if we update the mechanism, e.g. a key length, algorithm, protocol? Would two services be able to communicate with each other if one of them has the new version of the trust library, but not the other? If we are not touching this part, we are OK, but if we change something we have to roll-out the entire system (or subsystem) with all microservices. Note that this particular challenge can be handled differently, but that’s a topic for another post.

No-code reuse! Only service reuse will be allowed in our awesome microservice architecture.

Let’s explore that for a minute and for simplicity we will continue with the internal trust problem. If we are self-contained, we have dependencies to other services on API and protocol level, so if we change any of those we are pretty much in the same situation. And while in a monolithic system we can easily detect the dependency-hell, on API level in a distributed system it is much harder.

Conclusion

I am not saying that service-reuse is not a valuable tool for solving big composite systems problems, but we cannot solve one complexity with another complexity. We need to think about dependencies both on technical and on organizational level (e.g. how do we require a feature/bugfix for another team’s service), establish our aggregates carefully, have backward compatible APIs, use versioned APIs etc. Microservices give us a lot of agility, but from time to time we still need to think holistically about our system.

I will follow-up with another post that drills in the service-reuse concept..

References