When conducting a code review, a senior developer should ask themselves, “What are we optimizing for?” Having a clear hierarchy of developmental values across teams and organizations empowers developers and teams to build software that can withstand the test of time and the rigors of production.
I like to have a developer walk me through their solution, either with me face-to-face, or over a video conference. This allows me to get in their head and approach their solution from their point of view. The primary aspects I look for while the developers walk me through are.
- Understandability
- Debuggability
- When good enough is good enough
- Testing
Understandability
The first thing I look for is, “Can I look at this and immediately understand how this works?” From there, I want to ensure the code is readable and cohesive with the project’s architecture. Understandability consists of four aspects, which I refer to as the four Cs: conventions, comments, construction, and consistency.
Conventions
Variable names should be descriptive enough that a developer sitting down for the first time to look at a piece of code should be able to tell what they are. Ideally, we want a variable to be as short as possible while still being descriptive. Some suggestions include avoiding single-letter variables (with some exceptions) and avoiding stuttering, for instance, calling an item number Item.ItemNumber is less effective than calling that same item “Item.Number”. The most important aspect of naming conventions across a project or a team is to decide on a standard and stick to it.
Comments
Everyone who has spent time in the software industry has come across “Self-documenting code,” i.e., code written by developers who did not write any comments or documentation. Developers should include comments for blocks of code that are not necessarily self-evident and largely with a common sense approach. There may be large blocks of code without comments as well, and generally, the rule of thumb is the more complicated a piece of code is, the more comments should be present. As with other elements, teams and organizations should establish consistent guidelines for comment usage throughout their codebases.
Construction
Construction, in this case, means the architecture of the code. Senior developers should lean on their architectural experience to ensure that solutions are sound and consistent with the approach to similar problems across the team or organization. This means that the approach, the libraries used, the structure of the files, and more should all be consistent. This piece is also important as it requires more experience and is a great opportunity for senior developers to teach younger developers more about architecture.
Consistency
Consistency applies across many aspects of a codebase and is accomplished by establishing clear guidelines enforced by those performing code reviews. When new developers join a project, I frequently encounter situations where they want to overhaul the existing coding practices. Improving a codebase and the approach to a given problem is good as long as the changes are made consistently throughout. Guidelines should be created for low-level items such as naming conventions and comments and big things like architectural considerations and libraries. Depending on the company’s size, consistency may be applied at a team or company-wide level. Communication of standards and consistent application of them is critical regardless.
Debuggability
In the long run, one of the most important things we can optimize for in any software is the ability to recognize problems and fix them quickly in production. The practices mentioned above contribute significantly to this; solving issues that arise is much easier if the code is easily understood. Good logging is another major contributing factor to debuggability. Logs with too much noise make things harder to debug, so it is vital to be selective about what to log. My general rule is that problems should be logged once and only once. The information in that log should be sufficient to debug that problem. In addition, when CRUD operations are performed, they should be logged once with enough information to determine who did what and adequate information to recreate the operation, excluding any privileged information. To promote this, I encourage developers to avoid debuggers and rely more on the logs whenever possible. When logs are sufficient to solve problems in development, they are likely adequate to solve problems in production, and developers are used to identifying and solving problems with them.
When good enough is good enough
Most developers are problem solvers and enjoy the process of building software and the challenge of making it as good as possible. This propensity often puts developers at odds with business stakeholders because there needs to be a balance: developers want as much time as possible to write the perfect piece of software, and stakeholders wish to have that software delivered promptly and for it to solve their business problems. I consistently emphasize to our development team that our core mission is to overcome business challenges. It might be exciting to do a deep dive into code for weeks to squeeze extra performance out of it, but if that performance does not contribute meaningfully to the client’s business needs, it is not worth it. Developers must remember that once the business problem is solved and the code is clean, consistent, and debuggable, it is best to wait to optimize things until a business need arises.
Testing
The need for testing may seem obvious, but when working in a fast-paced environment with tight deadlines, ensuring everything written was tested – and tested well – can slip by the wayside. Sometimes, stakeholders need a feature out quickly, and developers may need more time to test adequately. Still, it is always important to take time to ensure the code is fully tested before its release. Generally, code should have unit tests and integration tests, and ideally, regression tests so developers can prove:
- Their code works by itself;
- Their code works as part of the whole;
- And their code doesn’t break anything else.
To achieve these, any software team needs structural standards for testing, and developers conducting code reviews must ensure that testing has been done properly.
Conclusion
There are many more aspects to code reviews than mentioned here, but these are the primary rules I try to remember regardless of the technologies. The purpose of code reviews is not just to look at what a developer has done to sign off on it but also to provide an excellent teaching/learning opportunity that is critically important to the professional development of green developers. Remember, no rule is absolute. Our role as engineers is to guarantee that when guidelines are bent, it’s done with discretion and purpose.
