Knowing what to review

Different aspects of code must be considered when you’re reviewing it. Primarily, the code being reviewed should only be the code that was modified by the programmer and submitted for review. That’s why you should aim to make small submissions often. Small amounts of code are much easier to review and comment on.

Let’s look at the different aspects a code reviewer should assess for a complete and thorough review.

The company’s coding guidelines and business requirement(s)

All code being reviewed should be checked against the company’s coding guidelines and the business requirement(s) the code is addressing. All new code should adhere to the latest coding standards and best practices employed by the company.

There are different types of business requirements. These requirements include those of the business and the user/stakeholder as well as functional and implementation requirements. Regardless of the type of requirement the code is addressing, it must be fully checked for correctness in meeting requirements.

For example, if the user/stakeholder requirement states that as a user, I want to add a new customer account, does the code under review meet all the conditions set out in this requirement? If the company’s coding guidelines stipulate that all code must include unit tests that test the normal flow and exceptional cases, then have all the required tests been implemented? If the answer to any of these questions is no, then the new code added by the developer fails and is sent back to be corrected.

Naming conventions

The code should be checked to see whether the naming conventions have been followed for the various code constructs, such as classes, interfaces, member variables, local variables, enumerations, and methods. Nobody likes cryptic names that are hard to decipher, especially if the code base is large.

Here are a couple of questions that a reviewer should ask:

• Are the names long enough to be human-readable and understandable?
• Are they meaningful concerning the intent of the code, but short enough to not irritate other programmers?

As the reviewer, you must be able to read the code and understand it. If the code is difficult to read and understand, then it needs to be refactored before being merged.

Formatting

Formatting goes a long way to making code easy to understand. Namespaces, braces, and indentation should be employed according to the guidelines, and the start and end of code blocks should be easily identifiable.

Again, here is a set of questions a reviewer should consider asking in their review:

• Is code to be indented using spaces or tabs?
• Has the correct amount of white space been employed?
• Are there any lines of code that are too long that should be spread over multiple lines?
• What about line breaks? Do the line breaks adhere to the rules laid out in the coding standards?
• Following the style guidelines, is there only one statement per line? Is there only one declaration per line?
• Are continuation lines correctly indented using one tab stop?
• Are methods separated by one line?
• Are multiple clauses that make up a single expression separated by parentheses?
• Are classes and methods clean and small, and do they only do the work they are meant to do?
• Do you see anything that stands out that, even if it compiles and works in isolation, could cause bugs when integrated into the system?

Testing

Tests must be understandable and cover a good subset of use cases. They must cover the normal paths of execution and exceptional use cases. When it comes to testing the code, the reviewer should check for the following:

• Has the programmer provided tests for all the code?
• Is there any untested code?
• Do all the tests work?
• Do any of the tests fail?

Let’s see how the process works:

Figure 2.11: Test plan process flow

Untested code has the potential to raise unexpected exceptions during testing and production. But just as bad as code that is not tested are tests that are not correct. This can lead to bugs that are hard to diagnose, can be annoying for the customer, and make more work for you further down the line. Bugs are technical debt and are looked upon negatively by the business. And so, as part of the process of developing code, you need to ensure you have proper unit and end-to-end tests, and that the inputs and outputs of those tests are correct.

Moreover, you may have written the code, but others may have to read it as they maintain and extend the project. It is always a good idea to provide some documentation for your colleagues.

Now, concerning the customer, how are they going to know where your features are and how to use them? Good documentation that is user-friendly is a good idea. And remember, not all your users may be technically savvy. So, cater to the less technical person who may need handholding, but do it without being patronizing.

As a technical authority reviewing the code, do you detect any code smells that may become a problem? If so, then you must flag, comment, and reject the pull request and get the programmer to resubmit their work.

As a reviewer, you should check that those exceptions are not used to control the program flow and that any errors that are raised have meaningful messages that are helpful to developers and to the customers who will receive them.

Documentation

Documentation is vital to the success of a software project. You need enough documentation to enable a full understanding of the system so that it makes extending and maintaining the existing software easier and less error-prone. Good documentation makes it easier to on-board new software developers and help them get up and running faster. Here are some things to ask when reviewing the documentation for a project:

• Is there adequate documentation of the code, including comments, documentation comments, tests, and customer product documentation?
• Is the code well documented to aid with maintenance and support as well as adding new product extensions?

Architectural guidelines and design patterns

When performing an architectural review of a C# project, you should be looking at the overall structure of the application and ensuring that it adheres to best practices and is scalable, maintainable, and has good performance. You should also ensure that the architecture supports the current and future requirements of the project.

Here are some questions you should ask yourself during the review:

1. Is the application using a layered architecture, such as the Model-View-Controller (MVC) pattern or the Model-View-ViewModel (MVVM) pattern?
2. Are the layers of the application properly separated, and are there clear boundaries between them?
3. Are there any circular dependencies between components or modules in the application?
4. Are the interfaces between components or modules well-defined and easy to understand?
5. Are there any potential performance bottlenecks, such as slow database queries or inefficient algorithms?
6. Are there any security vulnerabilities in the application, such as injection attacks or cross-site scripting (XSS) attacks?
7. Is the code organized and structured in a way that makes it easy to maintain and understand?
8. Are there any anti-patterns or bad coding practices in the application?
9. Are there any unnecessary dependencies or components that could be removed to simplify the architecture?
10. Does the architecture allow for easy testing and debugging of the application?
11. Is the code adhering to SOLID principles and other design patterns to ensure code quality and extensibility?
12. Are there any performance issues or bottlenecks that could be improved, such as network latency, database connection pooling, or caching?
13. Is the application following a consistent naming convention and code style to ensure that it’s easy to read and maintain by other developers?
14. Are there any dependencies or third-party libraries that are being used, and are they being properly managed and updated?
15. Is the architecture taking advantage of modern C# features, such as async/await and LINQ, to improve code readability and performance?
16. Is the application following any industry standards or best practices, such as OWASP guidelines for web security or Microsoft’s .NET Core design guidelines?
17. Are there any opportunities to improve the code’s organization and structure, such as grouping related code into namespaces or creating separate projects for different components of the application?
18. Are there any potential issues with deploying and scaling the application, such as difficulties with load balancing or scaling horizontally?
19. Are there any opportunities to introduce design patterns or architectural improvements to reduce complexity and improve maintainability?
20. Is there a clear separation of concerns between the different components and layers of the application, such as separating data access logic from business logic and presentation logic?

By addressing these questions during an architectural review, you can ensure that the C# application is well-designed, easy to maintain, and meets the requirements of its intended use case. This can lead to a more efficient development process, fewer bugs and issues, and improved user satisfaction.

Performance and security

Other things that may need to be considered include performance and security:

• How well does the code perform?
• Are there any bottlenecks that need to be addressed?
• Is the code programmed in such a way that it protects against SQL injection attacks and denial-of-service (DoS) attacks?
• Is code properly validated to keep the data clean so that only valid data gets stored in the database?
• Have you checked the user interface, documentation, and error messages for spelling mistakes?
• Have you encountered any magic numbers or hard-coded values?
• Is the configuration data correct?
• Have any secrets accidentally been checked in?

A comprehensive code review will encompass all the preceding aspects and their respective review parameters. But let’s find out when it is the right time to even be performing a code review.