Improving developer productivity and software quality via code reviews
Zone’s head of .NET development, Andy Butland, discusses the importance of organised code reviews for learning, knowledge-sharing and mentoring…
Looking back over some of the many ways we’ve tried to ensure high code quality in our solutions at Zone, I feel the introduction of organised code reviews was a key improvement in our practice. While we’ve always tried to ensure multiple sets of eyes on the code we produced, it became embedded as a technique along with the use of a technique called Gitflow as a branching strategy.
The reason that this method of treating source control worked so well with code reviews is that it provided a regular and natural point for such checks to take place. With Gitflow, features are worked on in individual branches of the source code, ideally for a fairly short period of time, before they are merged back into the main branch. It’s at that point a code review takes place, via a pull request (PR), by one, or ideally more, members of the development team. Only when reviewers, often in conversation with the original developer as we’ll discuss, have checked the code and approve the request, is the code for the new feature merged in. It’s then available for other developers to build on top of and for QA personnel to test.
Benefits of code reviews
Organising and carrying out code reviews in this way provide a number of benefits: some obvious, some less so. Clearly two sets of eyes are better than one and, unless using techniques like pair programming, the first version of a code update to solve a given problem will be the efforts of perhaps just one developer (or perhaps one of each different disciplines, e.g. “front-end” and “back-end”). Although not replacing testing, as often subtle bugs won’t be spotted by eye, it allows for a visual check that the code should function as intended, based on the PR description or linked ticket’s description. If time permits, the reviewer can also pull the branch down to their local environment to run it and confirm that it works as it should.
It’s perhaps more important at this stage though that the code review considers aspects wider than just the meeting of functional requirements. A reviewer will have an eye on security aspects, looking for any potential issues that could be an avenue for such a breach. Performance can be considered, by checking for any cases where clearly non-performant methods may have been used. And certainly a very important “-ility” to review is the maintainability of the code.
A major factor in code-base health for maintenance purposes is the recognition is that code is usually read much more often than it’s written, whether by other developers, or even the same developer coming back to it in a few months time, when all the details won’t be fresh in their head. A reviewer who hasn’t worked on the feature but has a high-level understanding of the requirement it is looking to meet is in the best place to see how legible the code is for someone coming fresh to it. They’ll likely look for naming issues — where code constructs like classes and variables aren’t clearly identified as to what they are for — and for modules of code that are too long or too complex to easily parse, and can be refactored into smaller, more easily comprehended pieces.
Often there’s more than one way to implement a given feature. Some will be clearly better than others, but in other cases it may come down to personal preference as to which is the best approach. Even in such cases though, it’s worth a reviewer considering the new code in light of existing patterns and practices used in the rest of the solution. There’s a balance to be struck between ensuring the code-base moves forward, taking advantage of modern techniques and ensuring consistency within the solution. Generally I’d lean toward avoiding “islands” of code that uses a distinct technique or syntax, even if it’s a more modern and better one, without also taking the time to modernise the rest of the solution too.
In addition to concerns of the specific code under review, implementing a healthy system of code reviews has wider benefits too. It encourages shared knowledge of the features that make up a given solution, avoiding concerns such as when one area is always considered a single developer’s fiefdom. If and when that person is no longer around, or just temporarily unavailable, it’s good to have others at least broadly familiar with the feature, who can make changes without fear of breaking something due to a lack of understanding. With code reviews in place, at worst case, at least two sets of eyes has seen every line of code committed. In also reduces the chance of a new implementation for a feature or function being created due to lack of knowledge that something similar is already in place.
Finally, above and beyond the application itself, code reviews are a great opportunity for learning, knowledge sharing and mentoring. I’ve certainly learned a lot from various reviewers’ comments of code I’ve written and in discussions on points I’ve raised for others. I’d like to think when the opportunity presents I’ve been able to pass on a few things too, which will be of benefit to the other developers in the discussion as they move on to other features, applications and roles.
Running a healthy code review process
To gain all the benefits above, it’s not just important that the practice of reviewing code is adhered to, but that the process is carried out in a healthy, non-adversarial and inclusive way. An important starting point for this is to ensure the rules for review are the same for all. In other words, code review isn’t about senior developers checking junior developers’ work; rather, everyone has their code reviewed, and everyone's opinion is of value. So whilst the code review remains a “gate” through which it has to pass before being accepted into the code-base, at this point it’s a collective responsibility to ensure the amend meets the standards the team aims for, and not a judgement on an individual (particularly if any issues that are identified are accepted and learnt from for future work).
In preparing the code for review, there’s a need for balance between the size of the PR and the productivity of the development team. Too large, and it can be hard to find other developers — who of course are likely working on their own features — with the time and inclination to do a thorough review. Too small, and we increase the interruptions, waiting time and chance of wasted work due to subsequent changes making the reviewed code obsolete. Fortunately, this same sizing issue is also considered when planning user stories of appropriate complexity for the development team to work on within an agile workflow, with stories that are too complicated being broken down into smaller ones. As such, one PR per story or bug fix is usually an appropriate measure.
With sensibly sized PRs, the rest of the development team should prioritise picking up the work for review. Not necessarily dropping what they are immediately doing, of course, but perhaps taking on a pending review whenever they have a sensible break point in their own feature work. By responding promptly, the time work is pending being merged into the main branch is reduced, meaning any waiting time to build further on the code being reviewed is also minimised. And this further encourages against PRs getting too large, as the developers don’t fall into the trap of “just adding one more thing” before they have to wait for the PR process to complete. That said, this certainly isn’t an argument to rush the review — rather finding an appropriate balance between speed and thoroughness.
One thing that can be a little depressing for a developer new to the team is a code review coming back with a lot of little, seemingly “nit-picky”, comments. These shouldn’t be ignored — even if they make no differences to the running of the code, to gain the benefits of consistency and legibility there’s value in the code within an application adhering to agreed conventions. However, rather than relying on humans to find and fix these, we can lean on tools here — development environments and linters — to check for these things in a local or CI environment and fix them up before the code review is submitted. By doing so, it allows reviewer and reviewee to concentrate on the more important things that automated tools won’t pick up.
Tools can also be used for tracking unit test coverage, ensuring that a reasonable percentage of code is exercised in the context of a unit test, verifying it’s correctness by automated means. There are judgement calls to make here though, trading off the effort and value of testing, so human input is still of value here — ensuring the tests submitted with the code are appropriate and accurate.
When it comes to more contentious issues — i.e. points on a review that don’t have a clear and obvious resolution — it’s important to remember that the review is a conversation and discussion, and a comment is a suggestion, not a direction. If the reviewee disagrees with a comment, and has good reasons for preferring the way they had it originally, they should certainly say so, and in many cases, with the additional context, the reviewer will likely agree.
Sometimes though, a reviewer may feel they have a better idea for a solution, but a comment just doesn’t cut it with getting the point over. It’s tricky to compare working, running code with something that’s, at this point, just an idea. Here they might consider, time permitting, making a “PR to the PR” — i.e. at least stub out in rough form the idea for the alternative implementation, and give the reviewee the opportunity to consider that. They may then either accept the PR (thus updating their version to the new one), manually amend their code, or simply still prefer their original approach. The lead on the feature remains the original developer, but it just maybe that this approach adds more value than a necessarily vaguer comment suggesting the improvement.
Lastly, one small things that I like to do, and appreciate if someone does for me, is to comment on the good things! Clearly the main reason for the code review is to improve the quality of the software, and so inevitably comments will focus on the things that aren’t in some way considered correct, but that doesn’t mean it’s not permitted to give a little virtual pat on the back for a particularly elegant solution or a general job well done.
