4 things to look for in a code review

Code reviews are one of the best things about being a software developer. They are the most amazing opportunity to help us grow as developers whilst generating respectful discourse between the reviewer and the reviewee. If you're new to code reviewing or haven't ever got the hang of what you're supposed to be looking for or asking, the below 4 questions help me structure code reviews. These have led to better overall software stability and upskilling every developer in my team (including myself).

Does it work?

This is the most fundamental question to ask about some code that is being reviewed. One would presume that if someone is asking you to review their code, then they must at least think it works. When we are reviewing code, we are not interested in whether or not we think the code works, but whether or not we know the code works. This can only be done by testing.

I'm not going to spend much time debating the merits of testing, but in the case of a code review, in my humble opinion, if there are no automated tests covering the code, then the assumption is that it does not work and the developer needs to go back and write some tests proving that their code works.

Write tests. Write tests often. Write tests everywhere. Write tests covering every conceivable eventuality. This is the most black and white part of a code review, the remainder is far more subjective.

Is it easy to understand?

I'm going butcher Kernighan's Law and apply it to writing code

Understanding code is twice as hard as writing code in the first place. Therefore if you write the code as cleverly as possible, you are by definition, not smart enough to understand it.

It's hard to give a clear definition of what easy to understand code is, but I like to apply the 2am rule to it. If I've been up all night and am trying to read some code at 2am, it had better be really simple, otherwise I'll have no idea how it works or what it does.

Make sure the naming is on point, don't call a variable "x" when it should be "age". Don't name a function "calculate" when it should be "calculate_current_bank_balance".

Try to limit cyclomatic complexity. This is just a fancy way of saying don't have too many branches in the code. This is best achieved by ensuring that every module or function does only one thing. Too many branches in the code mean that the code is more difficult to understand and more often than not, violates the single responsibility principle.

Does it follow existing patterns in the codebase?

If throughout the codebase, a repository pattern is being used for abstracting and accessing data, ensure that the code being reviewed doesn't directly access the data. This makes it much harder for new developers to come onto the project and get to grips with how the whole thing works.

A few well-structured patterns should be consistent and apparent through the codebase and be very easy to follow. If it's not readily apparent which patterns a developer is supposed to use in the codebase, this is probably a smell that the codebase does the same thing in a few different ways. This should be rectified to help all new code fall into the pit of success. Adding a description of these patterns in the README.md file will also go a long way to helping bring people up to speed faster.

In the case of relatively new codebases without established patterns, I'm a proponent of YAGNI, so am not overly opinionated with how things should be structured until a business/technical need presents itself. This is not to say the code should be written haphazardly, but generally, so long as it's achieving its proposed goal without breaking any existing architectural decisions, we should be good with it. It is still worth prompting a discussion around this topic though as these are the times where good positive discussions can happen which can really contribute to the professional development of all involved.

How has this PR improved the health of the codebase?

The way I think of my job, its that I am a custodian of the product that my company owns. This product in question is the codebase. One fundamental truth here is that every time we add code to a codebase, we're making it more difficult to understand. Adding more lines of code means that when someone is trying to understand what the code does, it takes them longer.

In the face of this ever-increasing complexity, what is this PR doing to reduce the complexity of the codebase? Is it improving some variable names? Is it adjusting some of the formatting? Has it refactored common functionality from a few different functions to a common function?

A good goal for every developer should be to leave the codebase at least as clean as you found it before making your change. Kudos for making it better.

Summary

For me, code reviews aren't a checklist (besides testing). They are an opportunity to guide and shape the codebase so that we limit the amount of technical debt we accumulate. It's like going on a hike. When you go out on a hike, take a map and compass and follow the routes laid out, whilst making sure you carry your litter back home. This way, next time someone wants to go on a hike through the same route, they're able to find the way (easily), and aren't swimming through litter in order to get to their destination.