Although I’ve written in the past on code reviews, it never hurts to revisit a great topic. Code reviews are a great practice to improve code quality and share knowledge on a team. I have been author, reviewer, and referee in quite a few code reviews in my career. Occasionally there is pushback in some reviews, where the author disputes the changes requested by the reviewer. In this post I put down some more notes on things to keep in mind during a code review.
I was listening to a podcast recently and one of the tips was to make sure that value is being added by the code review process. Make sure that before escalating an issue make sure you know what value you hope to get from that action. Also, have empathy. What value do you think your colleague is hoping to get from the same issue?
Make sure everyone knows why a code change is being requested. The answer shouldn’t be “just because that’s the code style I want”. Good code is written with a purpose. Everything about the code should be to make the code simpler and easier to maintain.
If there is ever a question about why one way is suggested to be better than another, be sure to understand why that is the case. Code reviews are about sharing your perspective with others and opening a dialogue. It is discovering and discussing different ways of doing things and improvement.
Code is produced by people. People can take offense to criticism of their code. Remember to make this a valuable discussion about code. Don’t make it about the skills of people.
Remember the respect must be mutual. It isn’t just the author that may feel hurt. The author being dismissive or overly defensive toward the reviewer can equally lead to friction.
Remember to keep the discussion about the code.
Although many code review issues are specific to the project at hand, some issues may be raised to encourage coding standards. Coding standards are important to the productivity of a team and the overall quality of all code.
For example, consider the problem of SQL Injection. Most developers are aware of this issue, yet it is still one of the most common security vulnerabilities in programs. How can this be?
Not all code that builds SQL strings manually suffers from SQL Injection. In some cases, the value being injected is coming from a constant or a limited set of constants (an enumerated type). Therefore, the argument could be made that in this case the extra work to make the code safe from injection is not needed.
Coding standards should exist so that injection vulnerabilities are always fixed, even when there is no immediate threat of injection. The constants might change in the future, which might break the SQL. However, the biggest problem is that the habit of writing injection-proof code is not being formed.
The reason that SQL injection exists is that developers often copy and paste code. Injection flaws are introduced when code that doesn’t need to be injection-proof is copied to a place where it is needed.
The solution is to just always write code that is injection-proof. Don’t have two coding standards for when it is and is not needed. This just makes the coding standards more complicated and harder to follow.
Having two standards to follow also creates a drag on productivity. The original author (and the reviewer, and every code maintainer) always need to constantly stop and think, how vulnerable is this code? That assessment takes time, which hurts the readability of the code. The team can be much more productive by not wasting time thinking about which standard to follow and always implement the highest standard.
If the only standard was to always write injection-proof code, then author (and readers) would be in the habit of seeing and working with that style of code. If injection-proof code really is much more difficult to write that just omitting it, forcing it to always be done will encourage helper functions or frameworks to be developed or used to make it easy.
Remember it isn’t always about, “the code works fine in this situation”. Coding just enough to get it working can sometimes lead to rather fragile code. Coding defensively and to a higher standard will yield robust code.