Code reviews are a critical part of ensuring code quality, maintainability, and adherence to best practices. The goal of this guide is to provide a structured approach to reviewing code effectively.
These tips are not strict rules but rather a helpful framework to focus your review efforts. They aim to ensure that the code is functional, scalable, and easy to understand, while also aligning with the team’s architectural standards and project requirements.
Tip
A key goal of these reviews is to establish a consistent coding culture where all team members write and review code in line with shared expectations. By defining these standards, we can:
- Create predictable, maintainable code.
- Foster collaboration and mutual learning.
- Reduce friction during development by avoiding unnecessary surprises.
As you review code, keep in mind:
- Collaborative mindset: The review is a conversation, not a critique. Provide constructive feedback.
- Value-driven focus: Prioritise changes that improve the code's quality, reliability, and maintainability.
- Continuous improvement: Use reviews as an opportunity to share knowledge and refine the team's practices.
Note
Automated Enforcement of Formatting and Type-Checking
Projects should establish a clear and agreed-upon set of rules for formatting, type-checking, and coding conventions. These rules should promote a cohesive "team coding culture" where code is predictable, adheres to established patterns and standards, and is easy to read and maintain.
To minimize subjective discussions during code reviews, these rules should be automatically enforced using tools for checking, validation, and, when possible, auto-fixing.
For example, whether an interface should be prefixed with I (e.g., interface IAsset {} vs. interface Asset {}) is less important than ensuring consistency across the codebase. What truly matters is that the team aligns on expectations and maintains a predictable coding style that everyone is accustomed to.
This means:
- No need to comment on issues like “this line is too long”, “spacing between brackets”, or other formatting-related concerns.
- Type safety is guaranteed by strict null checks, so there’s no need to manually verify types in most cases.
- You can focus your reviews on critical areas such as architecture, functionality, readability, and maintainability.
- Understand the purpose: Start by reviewing the ticket or user story that describes the change. Understand the problem it solves and the expected behaviour.
- Set architectural expectations: Think about the solution you’d expect to see based on the problem. Does the implementation align with what you'd expect in terms of simplicity, scalability, and adherence to project standards? If the solution is more complex than anticipated, consider whether the added complexity is justified.
- Single Responsibility: Does each class or function handle only one responsibility? Avoid multipurpose services or functions.
- Open/Closed Principle: Is the code open to extension but closed to modification? Check for hardcoded logic or structures that might make future changes harder.
- Dependency Inversion: Ensure high-level modules don’t depend on low-level ones directly; abstractions (interfaces) should be used. If Dependency Injection (DI) is part of the architecture, confirm it’s being used effectively.
- Coverage: Are the tests covering all critical paths, edge cases, and failure scenarios?
- Relevance: Do the tests reflect real-world use cases? Look for unnecessary or overly specific tests that don’t add value.
- Maintainability: Are the tests easy to understand and maintain? Check for overly complex test setups or assertions that could break unnecessarily.
- Clarity: Is the code easy to follow? Well-structured code should be self-explanatory without relying too much on comments.
- Naming: Are variables, functions, and classes named appropriately to reflect their purpose?
- Simplification: Is there any unnecessary complexity? Suggest breaking down overly large functions or simplifying convoluted logic.
- Explicit Types: Are types explicitly defined where they add clarity, especially for function parameters, return values, and interfaces?
- Avoid any: Check for overuse of the any type or poorly defined types. Consider replacing these with more specific or generic types.
- Avoid Unnecessary Complexity: Ensure that generics, types, or abstractions are used only when needed. Don’t introduce unnecessary complexity with over-engineered solutions. Keep the typing simple and intuitive, avoiding the use of generics unless they provide clear benefits in terms of flexibility or reusability.
- Modular Design: Are dependencies well-structured and logically grouped to promote clear separation of concerns? Ensure modules don’t have circular dependencies or unnecessary imports, as they can create tight coupling and make the code harder to maintain and test. Aim for a highly cohesive, loosely coupled design.
- Adhere to DI best practices: Make sure DI is implemented using recommended patterns and standard practices (e.g., constructor injection, use of interfaces/abstractions, avoiding service locators). This reduces tight coupling and improves the flexibility of the code.
- Efficiency: Does the code perform well under expected conditions? Look for potential bottlenecks (e.g., redundant database queries or inefficient loops).
- Scalability: Does the solution scale? If this code were used in a high-traffic scenario, would it hold up? Does it need a cache ?
- Input Validation: Are inputs properly validated (OpenApi) and sanitised?
- Secrets Management: Ensure sensitive data (e.g., API keys, passwords) is not hardcoded and follows best practices for secrets management (1Password).
-
Assess Library Necessity: Before adding any new third-party library, ensure that it addresses a clear and specific need. Avoid adding libraries for features that can be implemented with existing tools or custom solutions. Consider whether the library is well-supported, actively maintained, and widely used in the community.
-
Library Up-to-date: Check that the version of the library is up-to-date and installed with fixed version. Avoid unnecessary version mismatches that can cause instability or maintenance headaches.