Developing and maintaining web applications requires code changes of excellent quality. When a team develops an application for years, team members lose track of some updates. Developers tend to make more mistakes when touching code they are not familiar with.
Some companies have separate teams for development, testing and quality assurance. Keeping these tasks in the hands of developers is also an option worth exploring. Even if there is a QA team in your organization, the "QA should find nothing" mantra is worth preaching. Developer teams tend to perform better when they are in control of the safety net catching their own bugs.
Automated unit tests, integration tests and acceptance tests are mandatory for this setup. We should still not underestimate the role of manual code reviews. This article will introduce the elements of a solid code review process.
Testing and Code Review
For a thorough code review, you have to test the changes, so we won't draw a line between testing and code review. They come hand in hand.
Fault, Error, Failure
Code reviews will reveal a couple of bugs from time to time. Although other teams may catch some of these bugs, only developers can point out others.
Users of the system find failures which are noticeable manifestations of errors in a system. The cause of the errors are faults. While users notice failures, developers fix faults.
Some faults do not cause errors that propagate in such a way that the user faces a failure. Only developers tend to catch these bugs as no-one else can see them. The best way to detect these faults is an independent pair of eyes and focus.
Always focus on the cause of a problem. Treating symptoms is much like borrowing money. Technological debt can bankrupt your application.
The role of the independent reviewer
The reviewer is not allowed to take part in the implementation. Whoever implements code faces with bias associated with the knowledge of the implementation. Assuming equal complexities, people are more likely to find less errors in their own code than in the code of others.
If you are already familiar with the implementation, you are at a disadvantage. Some mental shortcuts may prevent you from noticing errors or formulating improvement suggestions.
Knowledge Transfer via Code Reviews
All sub-optimal solutions have a reason. This reason may reveal areas in which the implementer may get better. Discussing these areas makes the whole team better.
Correcting the violations of coding standards is useless. Enforcement of coding standards should be automatic. Shortcomings in the code should be shortcomings of the tools, not the people writing the code.
Naming of objects, methods, classes and other variables is an art. The team only gets better at it by thinking about improved names. Whenever the reviewer has trouble understanding a name, the team may come up with a better alternative.
Algorithmic efficiency and maintainability are other areas where the reviewer may contribute. Discussing alternative solutions makes the whole team better.
The 8 step process
The terminology of the below process assumes working with Git/GitHub. If you use different tools, this process still applies to you. Just make sure you know what the terminology stands for. The
master branch contains the source code in an identical state as on live. Feature branches contain the changes for the reviewer to address. GitHub issues describe a change. Each issue may include one or more commits.
- Preconditions. Determine what conditions are mandatory for you to review the change. For instance, the feature branch should be up to date with master. All automated tests should pass. Automated tests should cover the solution. If any of your conditions are missing, reject the update and reopen the issue.
- Read the issue. Make sure you are aware of the full scope of the problem.
- Reproduce the original state on the master branch. Keep this state open for further comparison.
- Test the implemented solution on the feature branch without looking at the source code. Verify that the behavior matches your expectations. Try to break the application in as many ways as possible. The solution should be robust. All bugs have to be noted. Bugs that can also be reproduced on master are out of scope if the fix is not implied by the issue description. Otherwise, the bug has to be fixed as an update should not introduce known bugs.
- High level review of the code. Read the source code of the diff of the feature branch and master. Understand the idea behind the solution. Contrast it with your own idea. Consult with the author whenever the thought process is not clear to you. Point out naming conventions that make it harder for you to understand the code. If a change is out of scope, suggest removing it from the feature.
- Second, detailed review. You should be aware of how the change should work. It is time to look at the details. Look for syntax and semantic errors in the code related to all changes. If you find out that other parts of the application are also affected, compare the behavior before and after the update. A thorough code review tests every modified object, method, using all possible boundary values. Think of everything that could go wrong. Whenever you test something that was not covered by tests, ask for better testing coverage. Always ask for fixing the cause, never settle with hiding the symptoms.
- Integrity check. Embrace DRY (Don't Repeat Yourself) principles. Whenever you can see that there is room for refactoring, point out the cause. Decide the exact scope with your team. More often than not, refactoring will be incremental. It makes sense to schedule these refactoring ideas as soon as possible though to save time.
Justification of the process
The code review significantly increases the quality of the solution. The team will learn how to include changes that are in the scope of the request, and exclude everything else.
Comparing the original state with the new state decreases the number of false alarms. The team discusses the efficiency and maintainability of the proposed solution. The process even includes formulating refactoring change requests.
The reviewer should be aware of two solutions: his own solution and the solution of the original implementer. What would happen if reviewers just read updates in isolation? They would miss the point of how the small changes contribute to the whole solution. Code review serves the purpose of reviewing how the written code solves the desired problem.
Is the review worth it?
Code review takes time. As time costs money, the question arises if we need code reviews at all. When it comes to a simple signature generator or a registration form for example, I would say that a code review is not necessary. There is little room for faults and most faults manifest in failures.
Developing complex software has different requirements. For instance, a banking software requires a solid development process, including strict code reviews. I won't give you advice on whether you need code reviews or not. If you do need code reviews, you may now know what to pay attention to to take your review process to the next level.