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 high quality code is easy to understand, easy to change, and correct. In that order.
Easy to understand
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. For example, you want to represent a money amount, so you code something like:
var debtInUSD = 200.15;
So if you familiar with USD you know that the decimal part refer to cents. How do you know we are dealing with USD? because the variable says so. Imagine if it were only something like:
var debt = 200.15
Could you tell what the currency is? is it USD or Euro? You probably would have to hunt down the code to figure that out. So you see, naming is very important when you try to make your code easy to understand. Don’t be lazy. Use meaningful names. Now consider the following example:
var debt = Money.USD(units:200, cents:15);
In this case, you know you are dealing with USD. At least at this point. If you find this later down the road you probably will have to hunt for the definition to see what are we dealing with. However, if you don’t care about the type, this should be enough (in the example being used here, you can think of USD and Euro as some kind of logical type, even if they’re only instances of the money type). Imagine the following:
var debt = Money.USD(units:200, cents:15); debt = debt.AsEuro();
In this scenario, including USD in the variable name would be misleading.
Easy to change
A code should be easy to change. You can think of this as a platform to add new features. The code should make it evident where to introduce the new feature. This requires constant refactoring to reflect our new knowledge in the codebase. The codebase itself should be a reflection of our current knowledge. There are many things that make a code hard to change, uncle bob classifies them in the following categories: rigidity, fragility, immobility and viscosity. At the heart of them lies the idea of coupling.
I often look for references that couple objects, modules and projects unnecesarily.
Correct
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 an easy to change codebase, I tend to end up with some code that is hard to understand. The reason for this, in my experience, is that we often introduce new levels of indirection in order to decouple the code, which in turn makes it harder to understand. So, by focusing on making the code easy to understand first, I can begin introducing indirection levels 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 all of these, but that’s a post for another time 😉
So, what do you look for when doing a code review? leave your comments below