The secret to great code reviews

Nowadays, lots of people in development use code review recognising it as both helpful and useful. Even if you don’t perform review you probably know what it is.

There are loads of code review tools available on the market, complete with use scenarios, recommendations and rules, such as: GitHub, Phabricator, FishEye/ Crucible, GitLab, Bitbucket, Upsource, to name but a few.

There is plenty of information available out there as well. A simple Google search brings up loads of articles about code review with real examples, first-hand reports and overviews of all the different approaches, all telling you what to do and not to do, what to bear in mind and what to ignore, etc. Basically, the subject has been pretty much done to death.

Which is precisely why the rest of the proverbial iceberg might go unnoticed.

I trust that you are serious about the things I am describing in this article, in some cases deliberately overstating the case. Review, like any other process, requires careful and thoughtful implementation. An approach along the lines of, “Let’s do it like everyone else; let’s just get it done,” is not only not going to work; it may even be damaging.

Having read this introduction you might be asking, “What’s so complicated about code review? It’s a piece of cake. Especially since most of the tools listed above offer a review flow right from the offset (open the ‘box’, install, commit/push — and off you go!).”

However, experience tells us that things are not quite as plain sailing as they may seem at first sight, due in part to its apparent simplicity. Once everything is up and running there is a risk that the manager relaxes and stops taking an active interest in the process, preferring to leave it with the developers. And the developers, for their part, will either pick up the process or be too busy on other things to give it their full attention — to the detriment of solving the problems that the review process is intended to resolve. The manager may not even realise that all is not well, because no instructions have been issued or rules enforces. Having said that, it is also very important for any business that problems get resolved as quickly as possible, and that time isn’t wasted on unplanned activity.

A long time ago, when I was working at another company, I remember a conversation about code review with one of the leading developers. It was PILOT. Maybe some readers will have heard of him (Hi, PILOT :)). He is working with us at Badoo now and that’s great.

Right, so this is what PILOT said at the time, “The most complicated thing about the review process for me is compromising and approving a finished task which is working correctly, even if I know there is another solution and even if I like my way better.” In my opinion this thought is key. And the main point of this whole article is basically along the lines of this principle.

Why do we need a code review process?

Do you have review at your company? Are you doing it correctly? These brief questions I’ve devised might make you think twice.

There are just three questions to ask yourself:

1. How much time does code review take for a hypothetical average task?

2. How are you reducing the review time required?

3. How do you know that the review for a specific task has been done correctly?

If you don’t have clear answers to some (or all) of these questions, or if you have doubts about your answers, then I venture to suggest that there is something wrong.

If you don’t know how much time is required for review and are not constantly reducing it, then how are you able to plan? Is it not possible that the person carrying out the four hours review was actually on a tea break for half that time? Can you be certain that the tea break isn’t getting longer and longer with each successive task? Maybe the reviewer isn’t looking at the code at all but is simply hitting “Code is OK”.

And, even if the review is being done conscientiously, how can you ensure that as the code base and company grow that the time required to perform the tasks is not increasing? After all, management generally expects processes to speed up, not slow down, right?

If the answer to the third question doesn’t immediately come to mind, it is worth asking yourself another question. Do you know why you actually need this process? Is it just because all the other ‘grown-ups’ are doing it? Have you considered that you might not need it at all?

Ask your leads and developers what it means to do code review ‘right’ and you will be surprised by all the different answers you get. They will vary according to personal experience and convictions, each person’s own inner sense as to whether something is being done correctly or not, and even in line with the technological stack and development language that has been used.

Remember those ready-to-use code review tools I mentioned earlier, with their own approaches and flow? I, for one, would be interested to hear what answers the developers of those tools would come up with. Would their answers as to the right way to do code review match your staff’s answers? I am not certain they would. Unfortunately, I sometimes observe people implementing review tools who never for one moment question their necessity. Either they are doing it merely to ‘tick the box’ or to be able to report back that they have implemented review and everything is fine. And then they just forget about it and move on.

I don’t want to be Captain Obvious, but are you listening to your staff? Are their answers along the lines of, “Because that’s what you do,” or “It’s best practice; everyone does it.”? As you all well know, there is no point mindlessly implementing a process without understanding why it is needed. Every process needs to be justified and then — if it is needed — implemented, taking into account the various needs of the business and the project, and also any problems which really need to be resolved. A cargo cult approach is hardly appropriate in a company with a good engineering culture.

Correspondingly, it is important for a manager to communicate to people what constitutes code review that’s ‘right’ for their project. And, prior to that, it is worth a manager formulating the criteria for ‘right’ code review, selecting suitable tools and laying down appropriate rules. That way you won’t be a million miles from performing successful monitoring.

Bad code review

So what is the right process? Let’s get to grips with this. If you decide to keep on reading, I have shared lots of my personal thoughts below. For those who are impatient to find out my reasons for writing all this, let me suggest you fast-forward to the section entitled, ‘Good review code’.

By the same token, my thanks go out to those who have stayed with me so far. Regardless of the points I make here, at the end of the day it’s always going to be a question of working out for yourselves what constitutes right review, based on the needs of your particular project and carefully thinking everything through. All I am going to do is to try to help you with this.

To identify for oneself the important and essential elements in any process, it’s sometimes helpful to start off by eliminating clearly unnecessary things which are damaging to engineering culture. This is how we shall proceed.

Sharing knowledge

In answer to the question, “Why do we need the code review process?” I have very often heard the answer, “To share knowledge”. Sharing knowledge is of course helpful and useful. And many will agree that it is important to have a process in the team for sharing knowledge and expertise. But are you sure that code review is the right tool for this?

On more than one occasion I have asked people what they mean by ‘sharing knowledge’ and got several different answers.

One answer is that it is about showing newbies (those who are new to the team or to the component in question) what the accepted rules and practices are: this is how we do it, this is how we don’t do it — and these are the reasons why.

A second answer is that it is about getting a fresh perspective from the newbies’ (again, those who are new to the team or to the component in question) on the way you normally do things. If your processes should be ineffective in any way, then the new person might just help us find this out.

A third answer is that code review is simply about becoming acquainted with part of the code. If, in future, the reviewer finds themselves needing to change any part of the code, it will be much easier for them to do so.

Let’s go through these points one by one and try to assess the degree to which they are appropriate as part of the review process.

Code review as newbie training tool

In this case we are mainly talking about a situation where a newbie is given a task, which they carry out, and then an experienced team member (the component owner) shows them what mistakes they have made. This is an important and necessary process, I am not contesting that. Newbies need to be shown the team’s established rules.

But let’s be honest: isn’t it a bit late? Wouldn’t it be better to tell a newbie about these rules before giving them access to the code? After all, mistakes can be very costly. And newbies rarely work the way you need them to straightaway, so, the task will keep being sent back to be redone. And that, in turn, means the product will be available to the user later than it should have been.

All in all, this approach amounts to taking a process with a lead time which was already difficult to estimate — and adding another process with an even more unpredictable lead time. Thus, we end up increasing the time required to deliver the task to the user by an even less predictable value.

Of course, on-the-job newbie training that uses work processes can have its place. However, it’s also true that we sometimes fail to see the flaws in approaches we are used to. And it is all too easy to make a mistake here.

For example, if there isn’t a process set up for training and coaching at the company, the manager will be forced to throw the newbie in at the metaphorical deep end. And the newbie won’t have a choice: it either sink (i.e. leave the company) or swim. There are people who thrive in these types of situations; others, sadly, can’t cope. I think a lot of professionals will be able to identify this scenario. My point is that this phenomenon can lead to precious company time being spent far from optimally. And, by the same token, the newbie adaptation process might not go as well as it should. And all this simply because nobody got round to organising an effective process for integrating new staff members onto the team and the manager thought that the newbie could learn everything at the code review stage. While in actual fact these are two different, very necessary and important processes.

Of course, even assuming the rules have been explained in advance to a newbie and that there are other training processes in place at the company, the newbie adaptation process does need be monitored in some way. Review is one of those processes which may help. But monitoring and training are not the same thing, are they? Especially as all team members need to be monitored — and not just newbies. After all, we can all make mistakes and forget about conventions and in one way or another this is going to affect the part of the system owned by the whole team (in this case, the code).

So, what conclusion can we draw? The process described above has a different aim: not training, but monitoring. And it is not only newbies but the whole team that requires this monitoring.

All this could be easily summed up in the following statement: “Code review is necessary to monitor compliance with the conventions and rules shared by all members of the team.” And newbie training in this case would amount to nothing other than artificially redefining an aim — which only serves to confuse people and sets the process heading in a different direction.

However, even this conclusion, seemingly formulated correctly, can lead us to hasty action. The code review stage occurs when the code has already been written. And it can be very expensive to correct a code written without regard for the shared rules. We need to be able to notify the worker as early as possible that something has gone wrong. And, for this reason, I hold that code review is often not best suited as a process for monitoring compliance with shared rules. In lots of cases testing for compliance with the relevant rules can and should be rescheduled into earlier stages.

For example, code formatting can be checked in the Version control system hooks (likewise the use of deprecated classes, conventions around the location of files in the relevant folders, the use of external dependencies and third-party libraries and more). Doing this minimises the time required for code review later on.

Let’s continue our discussion about newbies training in the context of the code review process, let’s draw an analogy. Imagine that you make, for example, cakes. Let’s say you have received an order for a cake for a VIP. Would you trust a newbie to review the finished cake? Would you be prepared for a novice baker to assume responsibility for the failure or success of your bakery at the wedding in question? Or is what you really want at this stage is for them to get acquainted with the accepted team rules (how to bake a cake base, make butter cream or soak a cherry in cognac)? It would obviously be rather odd for a newbie to be getting to know the rules or to be performing monitoring at this stage, after all, the cake has already been made. So when it comes to features and code why do we act any differently? Or don’t the failure or success of our company in the eyes of users and customers depend on the features we run?

You may protest at this point, saying that it isn’t every day that we work on orders for VIPs and that it is entirely okay to entrust a newbie with a non-urgent or non-important task. And you would be right.

However, first of all, what is a newbie really going to learn from carrying out easy tasks with a small number of changes in the code (the changes in question being in non-critical places and not that important from a business point-of-view, since the task is not urgent)? How are they going to learn to bake cakes if all they are doing is whipping cream all the time? Of course, you need to move on from simple tasks to more complicated ones, but we need to do so while carefully monitoring the process in question. Newbies need to be trained, not left to their own devices to learn for themselves.

Secondly, a company can incur damage even by adopting what appears to be a useful and right approach. This is very simple. Using the reductio ad absurdum method makes the conclusion as simple and understandable as possible.

Let’s imagine that we come out and say that the tasks the product manager is giving us are needed for newbie training. Why? Also, what about code review? After all, the reason we assign some simple and non-urgent tasks to newbies is for them to learn how to do things our way.

What can this lead to? It could lead to a zealous worker, one who performs their task conscientiously and considers that everything they do must be done as well as possible, being side-tracked to perform training. They might set a training task for the newbie to come up with several solutions to tasks instead of just one, in order, in due course, to show the trainee the advantages and drawbacks of various approaches. They might give talks, comparing approaches and best practices applied at the company and beyond. They might take on lots more besides with a view to training the newbie.

In the case of code review the zealous worker in question might do a number of things: they might initiate a protracted discussion in the review comments regarding the benefits and drawbacks of given solutions to tasks; post chunks of code from the Stack Overflow to show that different people have different ideas; and perhaps provide links to articles and books which the trainee needs to read in order to understand that their solution to a given task is not ideal. As a result, the time required to perform a task keeps on increasing, because our developers are focusing on training instead of development.

Any normal training process of course has a right to exist, but it needs to be done the right way. And it is far from always the case that the training process should be integrated into the process of accomplishing business tasks. The reason being that these are two different processes. Let a newbie make various parts of the cake, let them produce various versions, let them get things wrong and throw them away. Let someone more experienced show them what to do and pass on old recipes to them. But it is not ideal for them to be learning on the job when a product is being created for clients. But if you have decided to do this, then make sure you hold the newbie’s hand and don’t let them come up with things during the course of their training which are going to hinder the production process.

Unless your organisation is an institute, school or training college or another kind of educational institution, then training is not your direct responsibility. You have been hired to deal with other issues and to achieve different outcomes.

On that point, it is precisely for this reason that in the case of Codeisok we are not adding functionality for uploading pictures, formatting text or text illumination for the code in the comments, despite having received quite a few requests for these kind of features — and continuing to do so. Our position is that code review is not a personal blog, messaging service or training service. This tool is required for something different. After all, normal text comments are more than adequate for highlighting pointing out code defects.

Code review — a fresh angle on code

Moving on. The next scenario, ‘sharing experience’, is as follows. Our team is working on something, complying with internal conventions, and we have got used to doing things in a particular way. Along comes a new person (from another team or another component) and they are able to spot a drawback in the way our work is organised.

Good point. Sounds reasonable. Might we, in fact, be doing something wrong?

However, let’s get back to the task’s life cycle and the point in time at which the code review stage starts. Isn’t it a bit late? The cake has already been baked, the bases covered in cream and the decorative flowers attached. The price of making a mistake is very high. And then we find out that at another bakery, for effect, add bicarbonate of soda in lemon juice to the base of the cake. Now what do we do? Redo the cake?

Obviously not, since 1) we have always made it without bicarb and the outcome wasn’t bad; 2) perhaps customers buy cakes from us and not from the other bakery because they don’t like the taste of bicarb; and 3) the cake is already finished, the wedding is tomorrow evening and we haven’t got time to make a new cake.

So, what’s the point here? We need to share experience. We need a fresh perspective. For example, before baking bases, you can find out from the newbie, “Have you done this before? Why is this approach better?” And it is probably worth baking a couple of cakes both the old way and the new way, so that regular customers can try them and we can find out what they think.

The mindless implementation of best practices seen in articles and reports can be damaging both to the company and product. “All major players add bicarb to cake bases. Boogle, Tracebook, Unozone, Megahard — they all do it.” So what? Does that mean that a hypothetical Peter immediately has to jump to redo a task which has already been finished?

I am certain that it doesn’t. If, initially, prior to solving a task, you haven’t agreed that, “Cake bases contain bicarb,” then it wouldn’t be very wise to demand that it be added at the code review stage. Your business’s mission isn’t “to have bicarb in cake bases”. If your cakes sell fine anyway, it doesn’t matter at all whether they have bicarb in them or not.

We need to understand that sharing experience is a different process, in no way tied to code review. This other process may occur earlier, later or in parallel, but it is a different process. If you want to, why not arrange joint master classes with your competitors. Although someone might try to steal some top-secret recipe or some super-contemporary methodology for whipping cream using a drill. Maybe someone might try to sneak a look at other people’s ideas and perfect them using their own processes. But, anyway, surely it would be odd to do this on a metaphorical working production line, at the point at which your product is already virtually ready and the price of redoing it is very high?

After all, we are talking here about the review stage, review of code which has already been written and is ready for the next stage. This code is waiting for the reviewer to hit “Code is OK”. And your customers who placed the order are not at all prepared for you using a novice cook to prepare a cake which has already been baked, in order to show the novice cook how you bake cakes and request their feedback.

Code review as a way of becoming acquainted with part of the code

This is probably the most reasonable and understandable bit — and many would agree with it. This is the assumed scenario: a programme has written the code for a task, while the others have taken a look at it and studied it and now they know how to work with it. In this way we reduce the risk of the bus factor occurring: should the developer leave, others will have no problem working on their code. We are also ready for when someone else ‘gets their hands on’ this code later on– in which case, they will already know how to work on it.

But let’s think about this for a second: does this really work as intended and does it really help people to share competencies and knowledge of particular sections of code?

Let’s consider this scenario from the point of view of the person who wrote the code. Is it really the case that in every task they have used approaches and practices which need to be learnt by everyone on the team?

Let’s assume that a programmer, Vasya, has invented some kind of hot sorting algorithm (maybe they invented it a long time ago and have always used it or maybe they just invented it recently). Do all the team members need to know about this? Yes, of course they do. It is necessary for people to know that a programmer called Vasya has created an incredibly fast and effective sorting algorithm which will be useful for everyone.

However, in my view, this is rare. Frankly, there aren’t that many ‘hot’ things which absolutely have to be integrated into the shared rules and standards. And they are by no means used in every task. So, is it worth stopping the whole metaphorical conveyor belt each time, taking people away from their work to show them the code for all Vasya’s tasks?

Obviously, there are lots of companies where it isn’t possible for the whole company to learn about Vasya’s know-how at the review stage. After all, the whole team isn’t involved in doing review, but only a limited number of reviewers. If, in your case, everyone does review, even then you should still hold your fire: as the team grows it will become more and more inconvenient and eventually impossible. This is not a scalable process, and it is important to remember that the resources in terms of time that can be spent on code review are limited.

Now let’s look at the situation from the angle of the other developers who may be performing code review for Vasya’s code and assessing his work. Are they just sitting around with nothing to do and waiting for Vasya to show them what’s so hot about his code? In all probability, they are not; they have their own tasks to be getting on with. Everyone has their own things to do and developers are not always prepared to ditch everything at a moment’s notice to switch to discussing the achievements of their colleagues, however outstanding they may be.

And it would probably make more sense for Vasya to notify his colleagues of his invention in a somewhat different way. For example, he could make a presentation. Or he could simply write a message including the relevant section of code along with explanations as to why it is so ‘hot’ and why his algorithm really is that classy and why it should now be the standard for everyone else. The others can read it whenever they have time, ask questions and agree or not agree with the solutions in question. And only after all that will a decision be made to accept a new standard (or not, as the case may be). Possibly, someone else might offer a better suggestion, i.e. one which is more optimal, cheaper and faster to write.

See what I am getting at? What I am describing here is not a code review process at all, but rather a process for creating and approving practices and approaches. This is a different process entirely.

And at this stage colleagues may come up with responses such as, “At my last company we did it differently,” or, “I have read that is not the right way to do it.” So what? Best practices, generally speaking, at your average hypothetical company may be appropriate. Does that mean that they will all be useful at your company too? Not always by any means. And it certainly doesn’t require that a given person absolutely has to rewrite the code they have already have spent time writing.

Compliance with any sort of abstract or external best practice, guidelines, fads etc. — rules with are divorced from the company/context — for their own sake, it’s aboslutely no use to anyone. “Static methods are considered out of place,” “Nowadays you need to move that to Traits,” “Singleton is bad practice; it’s better to use factory and DI.” Says who?

Sometimes the sound arguments you hear are right for abstract projects and tasks, but not for real ones. Once the code has already been written it is too late to start discussing and recommending even if useful and helpful things, because they would be unjustifiably expensive to implement.

Moreover, it may be that Vasya the programmer’s code doesn’t contain any new hot algorithm or know how, but rather some ‘crutch’, a dirty hack which he had to use for whatever reason. Does everyone on the team really need to know about that? If the dirty hack is justified and there really wasn’t any other way of doing it, then yes. But, even in that case, it is not worth distracting the whole team and bringing the metaphorical production line to a halt. I doubt whether even Vasya the programmer himself would want that to happen.

First of all, there are not that many people who would be prepared ‘right now’ to be distracted by assessing the dirty hack in question, simply because everyone is busy with their own tasks. And, secondly, nobody will remember anything anyway. What’s more, there is a turnover of people on the team and if, in future, someone were to stumble over that hack, one way or another they are going to ask, “Why was it done this way?” And so the optimal solution would be to add a comment to the code itself. The comment would explain the reason which made Vasya the programmer resort to a hack of this kind, anyway. (Remember the rules as to what is appropriate when commenting: you shouldn’t be describing what you are doing in the code, but why it is that you are doing it).

And if using the dirty hack in question could have been avoided, obviously it is worth reopening the task and getting Vasya the programmer to redo it. This is the exactly what code review is intended to achieve and, it goes without saying, it doesn’t necessitate the whole team becoming acquainted with the code (what is the point of everyone learning about code which is going to be rewritten anyway?).

The third case is where a task includes neither anything super new nor any dirty hacks. It’s based on all the team’s accepted principles and rules, on the team, formulated as per shared standards etc. In this case what is the point of distracting the whole team with this task? After all, there is no bus factor, and, if someone else needs to work on this section of the code in future, they will be able to work it out fine without any trouble.

And finally, if I still haven’t convinced you, I have one more question for you. How can you discern whether knowledge sharing has genuinely occurred during the code review process or not?

— Peter, have you read the code the programmer Vasya wrote?

— Yes, I have.

— Did you understand it?

— Yes, I understood it all.

And that sums up the whole verification process. What makes you so sure that Peter has really understood the code correctly? Where is the certainty that, in future, when Peter changes that section of the code, he won’t do something drastic? How can you measure the time required for developers to grasp a given task, when one of the developers hasn’t seen the code before and the other one has reviewed the code in question a couple of times?

Moreover, who’s to say that next time Peter, who performed the code review, won’t be working on this section of the code, but Edward, who is currently doing the code review for Arseny the programmer working on an entirely different component?

So, my contention is that the task of acquainting people with code from other components during the process of code review works far from the way you would expect it to. It simply slows down the code review stage and, as a result, delays delivery of the feature to the user.

If it is essential to share knowledge between various components it’s better to treat it as a task separate, independently task: following the shared rules for the whole company; using the same technological stack; sharing knowledge at internal meetups and seminars; writing shared documentation (again, based on accepted standards and rules); assigning tasks vis-à-vis one set of components for them to be performed by colleagues working on other components. And all these things while following all the rules for newbie adaptation: telling them the way things are done at the company, what to do, what not to do, helping them put together a good technical plan etc. This sort of ‘immersion’ vis-à-vis a given component is much more useful for everyone involved than simply looking at the code.

Code review is purely a verification/monitoring process, a process which needs to be completed as fast as possible. And it is simply not appropriate to bundle it with other processes (newbie training, knowledge sharing, showing off expertise etc.). At the very least to do so makes it unjustifiably expensive and, most importantly, it prevents it working effectively at the stage in questions.

Code review as documentation

Another possible answer to the question, “Why do code review?” was in respect of documentation. In this case it might not be so obvious what is meant, so allow me to clarify.

Here is the scenario: a developer and a code reviewer have agreed on something in the review comments and, in future, if someone asks why in a given place something has been done they are able to find the relevant information in the review. In other words, use git blame to find the commit, use the commit to find the ticket, use the ticket to find the review, find the discussion in the review, study it and see why it has been done the way it has.

So, it works out that a developer who’s already working with lots of sources of information — standards and conventions, technical brief, ticket in the bug tracker, the code itself (with comments or without) — now has to delve into yet another one. And they need to read the whole thread, trying to catch the gist of the conversation and understand the reasons which prompted things to be done the way they have been. Not only that, the views of the participants in the thread in question may also change several times as it goes along. Sounds odd, doesn’t it?

However, questions may indeed arise, so what should we do in such cases?

Obviously, if a question has come up at the code review stage we do need to answer it. And if the reviewer has a question then we have to think that a similar question could be raised by people working on the code at a later stage. This may be a sign that the code has been written ignoring various conventions (and the reasons for that need to be clarified). In that case it is worth leaving a clarification in the code itself, for example, in the form of a comment with a link to the ticket. Then anyone, looking at the code, will be able to understand why it has been done the way it has.

If there aren’t any legitimate reasons for the conventions having been ignored, then the task obviously needs to be sent back to be redone and brought into compliance. Is it worth retaining the correspondence history on what happened and make future developers reread it? I think that is unnecessary.

Moreover, it may be that the code has been written in a way which is not sufficiently understandable for the reviewer, despite complying with the rules. In this case it should be rewritten to make it understandable to both the reviewer and future developers. Again, is it necessary to retain the history for future generations? I am not sure it is.

Even if a situation has arisen in which comments are needed and it is worth future generations knowing about it, even in this case I believe it is better to leave the comments in the code or in the ticket — i.e. in the sources of information which the developer already has. Why add a new one (especially, if there’s the whole correspondence history besides)?

In the ticket you could write, “It was decided to do A for reason B,” i.e. the outcome of all the discussions. This means that there’s less to read, any developer who’s interested won’t waste a lot of time, and, where necessary, they’ll still have access to the additional information they need.

The conclusion is self-evident: it is not worth using the review code as a means of documentation. Code review’s purpose is different.

Good code review

The list of tasks and processes which people are mistakenly prone to integrate into code review could go on and on. And even were I to try to compile a full list, it wouldn’t be exhaustive. After all, each person has their own opinion.

I think that in the present case we need to remember bundling several processes together isn’t always the right way of going about things. Especially if we are talking about processes with lead times which are complicated to plan and monitor.

Code review done correctly is a monitoring process: monitoring that the task has been completed in full; monitoring to check that all the shared rules and conventions have been followed; monitoring that the solution is not superfluous; and monitoring that will be easy to maintain and develop in future.

Therefore, I would like to draw attention to the following areas which absolutely must be monitored at the review stage:

Solution architecture and design

To what extent does the proposed solution correspond to the task? To what extent is it complicated to understand, maintain and/or expand? To what extent is the resulting architecture demanding on resources and how complicated are the algorithms realised? Is it possible to make it simpler, easier-to-understand or cheaper? Is there anything superfluous which isn’t related to the task? Are we using external dependencies and libraries? How justified is this? And so on.

We often forget that this is the monitoring stage. Therefore, the solution architecture should have been agreed on beforehand. At this stage we should be making sure that the conventions in question are being followed.

It is also worth bearing in mind that this is one of the most complicated, demanding and labour-intensive moments in code review. The person acting as reviewer needs to be prepared to make compromises and to remember that there are no ideal solutions. And if, in the task, they are seeing a solution which for one or other reason they consider to be less than ideal, then first and foremost they need to assess how the solution measures up to the needs of the business rather than to their own perfectionism.

A reviewer may propose their own solution, which they consider preferable (more ‘correct’) from the point-of-view of best practices, patterns and other things. However, it is very important to remember that the solution they see in the code is already better than their own solution in at least one important respect: namely, that it is finished. It has already been written. A developer has spent their time writing it, integrating the chunk of code with other elements, testing and writing autotests and so on. And, so, code should be corrected only when the solution in question is manifestly wrong.

Following accepted team rules and conventions

Let’s face it, We are all different. Everyone has their own perspective on things and you can often observe how, even in an established team which has been working together for a long time, people may hold diametrically opposing views. One person thinks that tables are better than spacing, while another thinks the opposite.

To avoid the hindrance of everyday tasks that this can cause, and so that every team member can get to grips with someone else’s code and task as fast as possible, team conventions are established. These cover everything from standards as to the presentation of code and project file and folder structures, to the formalisation of API methods and bans on using this or that language construction. In this way, a means of code maintenance is provided and the risk of bus factor reduced — and much more besides.

Once again it is important to remember that at the code review stage we monitor compliance with shared conventions. By now, it is already too late and may be unjustifiably expensive to be teaching coding standards.

Correctness of a given solution

Is a given solution displaying any defects? Are there typos in the code, magic numbers and other undefined constants? Are there magic strings and other unexpected data which the programmer has not taken into account when solving the task? What about null checks and other places where there is the potential to ‘shoot oneself in the foot’? Has the solution been checked for information security? And so on.

This is a very important part of code review and it must be performed very painstakingly and carefully. How many programmers on your team know that this has to be done at the code review stage? How many programmers actually do it?

Code testability and availability of unit tests

This testing stage is very important because it is intended to improve the infamous code maintainability.

Lots of people understand what the term ‘code maintainability’ means, but far from everyone is able to explain what it is. And since that is the case, how, then, can you set the team a task around maintainability? Rather, I believe that a developer who has given thought to how their code will test, particularly if they are testing their own code and have written an autotest for automating testing, will naturally strive for their code to fulfil most of the code maintainability criteria. After all, not doing so makes it is practically impossible to write an autotest.

A general recommendation in the case of any change to the code would also be to break tasks down in the optimum way. The smaller the volume of code passing through the metaphorical conveyer of processes from idea to production, the easier it is going to be to check for compliance, to test, and to combine with the code for other tasks and to upload. Code review is no exception. It is very hard to read and understand a task with a large quantity of changed strings in lots of files (and to keep in mind how they are interconnected). The risk and cost of correcting oversights may be very high.

Conclusion

Basically, that is it. These four points are those things which absolutely have to be checked at the code review stage. They are easy to formalise, easy to communicate to workers, and so it isn’t complicated to formulate verification criteria and predict lead times. Automation and other ways of cutting down review time become more than feasible.

And here are a few more pieces of advice to finish off with.

It is important to remember that any time that code is sent back to be redone it has a negative impact on the quality of realisation. Even if everyone is acting with the best intentions. In the case of code review this is how it works. The developer corrects the code but doesn’t test it as painstakingly as they did initially (what’s the point? They have already checked it and only made very minor changes). Experience shows us that in such situations bugs often occur precisely where corrections have been made in the light of code review. That is how human psychology works.

At the start of the article I suggested you answer three questions as to the necessity of code review and how to do it right. Now you have an algorithm to help you answer those questions. Knowing the monitoring steps and bearing in mind the size of the task, it is entirely feasible for you to be able to plan the time required for code review, and to shorten it more and more each time.

When implementing any process, it is important to remember the goals we have set and the problems we are aiming to solve. This make it much easier to separate the proverbial wheat from the tares and to formulate measurable criteria for assessing effectiveness. And you need to formulate these for yourselves and for your team. The team also very much needs to solve the problems which have arisen but may each understand them in their own way.

And one final important point. The processes, rules and values which are right for one company may not be as useful and workable at another. What I have described, in terms of examples of review being done the right way, works for our company. We’re all about rapid development and frequent releases and we perform review for each task. Think about whether that applies to your project and also about how review is one of those processes which works best when not left to its own devices.

But, of course, the method you adopt is up to you.