Quality code pillars: a guide to better code reviews

As a code mentor and member of a software development team, I’m subject to and carry on code reviews. However, I have noticed that many times people doing the review don’t have a clear idea of what to look for. This leads to discussions on stuff like style and micro-optimizations. Having been there myself, I would like to offer some ideas on things that you could look for when doing a code review. I want to share my personal quality standard. To me, a code’s quality is measured by 3 aspects: understandability, malleability, and correctness.

Understandability

Understandability measures how easy is for us to comprehend something. In one side we have simplicity, that is, easy to understand stuff. On the other side, we have complexity, or things that are hard to understand.

A good quality code is a simple one. It’s important to notice that there’s essential complexity (the inherent problem complexity) and accidental complexity (the solution complexity). I’m referring to the latter. Strive to keep your solutions simple. I have found that communication is the main idea here: does the code communicate the ideas succinctly?

When reviewing, I look for code that is poorly encapsulated or named. Also pay attention to the semantic distance between the concept and the symbol in the code that represents it, it may reveal leaking abstractions.

Malleability

From The Titi Tudorancea Library:

The property of being physically malleable; the property of something that can be worked or hammered or shaped without breaking

Metaphorically speaking, we refer to how flexible, how easy to change our code is. That is different than how easy is to understand. One is a mental act and the other a physical act. We can think of malleability as a continuum with flexibility in one end and rigidity in the other.

A rigid code is better explained by uncle bob:

Rigidity is the tendency for software to be difficult to change, even in simple ways. A design is rigid if a single change causes a cascade of subsequent changes in dependent modules. The more modules that must be changed, the more rigid the design.

I often look for references that couple modules, objects and projects unnecesarily.

Correctness

What I mean by this is as free of errors as possible.

Typically, we deal with 3 types of errors: syntactical, semantical and runtime.
Since the compiler usually handles syntatical errors, let’s focus semantical and runtime errors.

Semantical errors are related to the business logic. This is a moving target since the rules the software try to model tend to change over time (at least this is true for a line of business application). We usually detect them using unit testing and acceptance/functional testing.

Runtime errors are usually related to resources used by the application. You can detect these using integration, load and any other kind of tests that exercise the application resources.

If the tests related to the code piece I’m reviewing are not present, I ask the developer for any.

Closing thoughts

So, there you have it. I would like to say that the order in which these appear is the priority order for me i.e I’ve found that if I start trying to create a flexible codebase, I tend to end up with some complex code. My suspicion for this phenomenon is that often flexibility is attained through indirection, which makes the code harder to understand (complex). So, by focusing on simplicity first, I can begin introducing flexibility as this becomes necessary and still have a codebase that a new developer can pick up rather quickly. And if you have a code that is easy to understand and easy to change, you can easily correct it.

By the way, TDD promotes simple, flexible, and correct code, but that’s a post for another time 😉

So, what do you look for when doing a code review? leave your comments below