Code quality always matters, but is especially important if your code is part of a team project or is accessible to others. Regular peer code reviews have a vital part to play in the creation of high-quality, fit for purpose code that meets business needs and conforms to policy requirements.
Imagine a world in which every developer was free to code as they pleased. Doubtless we would find the same kind of code being written in many different ways, and ultimately our codebase would become an unwieldy mess. That’s why it’s important to have a coding standards policy that outlines the company’s coding practices and its code review procedures.
When code reviews are carried out, colleagues review each other’s code. They must understand that it is only human to make mistakes. They check the code for mistakes, coding that is contrary to company coding policy, and any code that, while syntactically correct, could be improved upon in the interests of readability, maintainability, or performance. Our book Clean Code in C# addresses these topics.
In this overview we will cover the following topics:
· Preparing code for review
· Leading a code review
· Knowing what to review
· Knowing when to send code for review
· Providing and responding to review feedback
Before we dive in, let’s describe the general code review process.
The code review process
Before peer code review you must ensure that your code compiles and meets known requirements. It should pass all unit tests and end-to-end tests. Once you are confident that you can compile, test, and run your code successfully, you can check it into the current working branch. Once checked in, you will then issue a pull request.
A peer reviewer will then review your code and share comments and feedback. If your code passes the code review, your code review is completed and you can merge your working branch into the main trunk. Otherwise, the code will be rejected, and you will be required to review your work and address the issues raised by your reviewer.
The following diagram shows the process:
Preparing code for review
Preparing for a code review can sometimes feel like a nuisance, but it does result in better code that is easy to read and maintain. Time spent preparing for code review can save the reviewer considerable time and make the review itself more effective.
Here are some points to bear in mind when preparing for code review:
· Always keep the code review in mind: When beginning any programming, you should have the code review in mind. Keep your code small. If possible, limit your code to one feature.
· Make sure that all your tests pass even if your code builds: If your code builds but you have failing tests, then deal immediately with them before moving on. Releasing code that ‘works’ but was a test fail could result in some very unhappy customers when the code goes to production.
· Remember YAGNI (‘You Aren’t Gonna Need It’): As you code, make sure to only add code that is necessary to meet the requirement or feature you are working on.
· Check for duplicate code: If your code must be object-oriented and be DRY (‘Don’t Repeat Yourself’) and SOLID (see Robert Martin’s 2009 paper Design Principles and Design Patterns) then review your own code to see whether it contains any procedural or duplicate code. If it does, take the time to refactor it so that it is object-oriented, DRY, and SOLID.
· Use static analyzers: Static code analyzers that have been configured to enforce your company’s best practices will check your code and highlight any issues are encountered. Don’t ignore information and warnings which could cause you issues further down the line.
Leading a code review
When leading code reviews, it is important to have the right people present, by agreement with the project manager. The programmer(s) responsible for submitting the code for review will be present at the code review unless they work remotely. In the case of remote working, the reviewer will review the code and either accept the pull request, decline the pull request, or send the developer some questions to be answered before taking any further action.
A suitable lead for a code review should:
· Be a technical authority: They should understand the company’s coding guidelines and software development methodologies. It is also important that they have a good overall understanding of the software under review.
Have good soft skills: They should be a warm and encouraging individual who is able to provide constructive feedback, so as to avoid conflict between the reviewer and the person whose code is being reviewed.
· Not be overly critical: They should not be over-critical and must be able to explain their critique of the code. It is useful if they have been exposed to different programming styles, and can view the code objectively to ensure that it meets the project’s requirements.
Peer code reviews are typically carried out on pull requests in the version control tool being used by the team. A programmer will submit the code to version control and then issue a pull request. The peer code reviewer will then review the code in the pull request. Feedback will be in the form of comments attached to the pull request. If there are problems with the pull request, then the reviewer will reject the change request and comment on the issues that need to be addressed by the programmer. If the code review is successful, then the reviewer may add a comment providing positive feedback, merge the pull request, and close it.
Programmers will need to note any comments made by the reviewer and take them on board. If the code needs to be resubmitted, then the programmer will need to ensure that all the reviewer’s comments have been addressed prior to resubmitting.
It’s a good idea to keep code reviews short, and to not review too many lines at any one time.
Effects of feedback on reviewees
When reviewing your peers’ code you must strive to ensure that feedback is positive. Negative feedback focuses on the reviewee rather than on the problem, and has a damaging impact on the reviewee. A lack of motivation then develops which can negatively impact the team, as work is not done on time or to the required level. Any bad feelings between the reviewer and the reviewee will also be felt by the team. The overall project is likely to suffer as a result.
If it gets to the point where the reviewee has had enough and leaves for a new position somewhere else then the project will suffers time-wise as well as financially, as time and money will need to be spent on finding and training a replacement. The following diagram shows negative feedback from the reviewer toward the reviewee:
Positive feedback from the reviewer to the reviewee has the opposite effect. When the reviewer provides positive feedback to the reviewee, they focus on the problem and not the person. They explain why the code submitted is not good, and will suggest to the reviewee ways in which it can be improved. The feedback provided by the reviewer is only done to improve the quality of the code submitted by the reviewee.
When the reviewee receives positive (constructive) feedback, it is easier for them to take comments on board and respond in the appropriate manner by answering the reviewer’s questions, and ask any relevant questions themselves. The code is therefore amended and resubmitted for review on the basis of a better understanding of the requirement. This has a positive impact on the team as the atmosphere remains a positive one, and work is done on time and to the required quality. The following diagram shows this:
The point to remember is that your aim as a reviewer is to be constructive, not destructive. A happy team is a productive team!
A useful technique for positive criticism is the feedback sandwich technique. You start with praise on the good points, then provide constructive criticism, and then finish with further praise. This technique can be very useful if you have members on the team who don’t react well to criticism. Your soft skills in dealing with people are just as important as your software skills in delivering quality code.
Let’s now consider what we should review.
Knowing what to review
A complete and thorough code review takes into account a number of different aspects of the code:
Company’s coding guidelines and business requirement(s)
All new code should adhere to the coding standards and best practices employed by the company. It should also meet business 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 code must be commented on, the comments addressed by the programmer, and the code resubmitted.
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. Names should be long enough to be human-readable and understandable, and be meaningful in relation to the intent of the code.
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 used?
· Are there any lines of code that are too long that should be spread over multiple lines?
· What about line breaks?
· Following the style guidelines, is there only one statement per line? Is there only one declaration per line?
· Are continuation lines correctly indented?
· 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?
Tests must be understandable and cover a good subset of use cases. They must cover the normal paths of execution and exceptional use cases. 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?
· Is there adequate documentation of the code, including comments, documentation comments, tests, and product documentation?
· Do you see anything that stands out that, even if it compiles and works in isolation, could cause bugs when integrated into the system?
· Is the code well documented to aid maintenance and support?
Let’s see how the process goes:
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 looked upon negatively by the business. 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.
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 raised have meaningful messages that are helpful to developers and to the customers who will receive them.
Architectural guidelines and design patterns
The new code must be checked to see whether it conforms to the architectural guidelines for the project. The code should follow any coding paradigms that the company employs, such as SOLID, DRY, YAGNI, and OOP. In addition, where possible, the code should employ suitable design patterns.
This is where the Gang-of-Four (GoF) patterns come into play. The GOF comprises four authors of a C++ book called Design Patterns: Elements of Reusable Object-Oriented Software. The authors were Erich Gamma, Richard Helm, Ralph Johnson, and John Vlissides.
Today, their design patterns are heavily used in most, if not all, object-oriented programming languages. Useful references are .NET Design Patterns, by Praseen Pai and Shine Xavier (Packt) and https://www.dofactory.com/net/design-patternshttps. The latter site covers each of the GoF patterns and provides the definition, UML class diagram, participants, structural code, and some real-world code for the patterns.
The code should also be properly organized and placed in the correct namespace and module. Check whether the code is too simplistic, or conversely is over-engineered.
Performance and security
A basic performance and security checklist includes the following:
· How well does the code perform?
· Are there any bottlenecks that need to be addressed?
· Is the code programmed in a way that protects against SQL injection attacks and denial-of-service attacks?
· Is code properly validated to ensure 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. But when is the right time to carry out a code review?
Knowing when to send code for review
Code reviews should take place when the development is complete and before the programmer of the code passes the code on to the QA department. Before any code is checked into version control, all the code should build and run without errors, warnings, or information.
When your code is complete and fully documented, your tests work, and your system integration all works without any issues, then you are ready to submit your code for a peer code review. Once your code is approved it can then be passed to the QA department. The following diagram shows the Software Development Life Cycle (SDLC) from the development of the code through to the end of the life of the code:
If the code review passes then the code is deployed to the QA team, who carry out their own internal testing. Any bugs found are raised for the developers to fix. If the internal testing passes QA, then it is deployed into User Acceptance Testing (UAT).
If UAT fails, then bugs are raised with the DevOps team, who could be developers or infrastructure. If UAT passes QA, then it is deployed to staging. Staging is the team responsible for deploying the
product in the production environment. When the software is in the hands of the customer, they raise a bug report if they encounter any bugs. Developers then work on fixing the customer’s bugs, and the process is restarted. Once the product reaches the end of its life it is retired from service.
Providing and responding to review feedback
It is worth remembering that code reviews are aimed at improving the overall quality of code in keeping with the company’s guidelines. Feedback, therefore, should be constructive and not used as an excuse to put down or embarrass a colleague. Similarly, reviewer feedback should not be taken personally, and responses to the reviewer should focus on suitable action and explanation.
Providing feedback as a reviewer
Workplace bullying can be a problem, and programming environments are not immune. Nobody likes a cocky programmer who thinks they are big. So, it is important that the reviewer has good soft skills and is very diplomatic. Bear in mind that some people can easily be offended and take things the wrong way. Knowing who you are dealing with and how they are likely to respond will help you choose your approach carefully.
As the peer code reviewer, you will be responsible for understanding the requirements and making sure the code meets them. Seek to answer these questions:
· Are you able to read and understand the code?
· Can you see any potential bugs?
· Have any trade-offs been made, and if so, why?
· Do the trade-offs incur any technical debt that will need to be factored into the project further down the line?
Once your review is complete, you have three categories of feedback to choose from: positive, optional, and critical. With positive feedback, you can provide commendations on what the programmer has done really well. This is a good way to bolster morale as it can often run low in programming teams. Optional feedback can be very useful in helping computer programmers to hone their programming skills in line with the company guidelines, and they can work to improve the overall wellbeing of the software being developed.
Finally, we have critical feedback. Critical feedback is necessary for any problems that have been identified and must be addressed before the code can be accepted and passed on to the QA department. This is the feedback where you will need to choose your words carefully to avoid offending anyone. It is important that your critical comments address the specific issue being raised with valid reasons to support the feedback.
Responding to feedback as a reviewee
As the reviewee programmer, you must effectively communicate the background of your code to your reviewer. You can help them by making small commits. Small amounts of code are much easier to review than large amounts of code. The more code being reviewed, the easier it is for things to be
missed and slip through the net. While you are waiting for your code to be reviewed, you must not make further changes to it.
The feedback you receive may be positive, optional, or critical. The positive feedback works to boost your confidence in the project as well as your morale. Build on it and continue with your good practices. You may choose to act or not upon optional feedback, but it’s always a good idea to talk it through with your reviewer.
Critical feedback should be taken seriously and acted upon, as this feedback is imperative for the very success of the project. It is very important that you handle critical feedback in a polite and professional manner. Don’t allow yourself to be offended by the reviewer’s comments: they are not meant to be personal. This is especially important for new programmers, and programmers who lack confidence.
As soon as you receive your reviewer’s feedback, act upon it, and make sure that you discuss it with them as necessary.
Code review has a key part to play in the production of efficient code that meets business requirements. Effective code reviews place demands on both reviewers and reviewees. As reviewers, we need to deploy our soft skills when providing feedback to our fellow programmers. As reviewees, we need to take positive feedback on board and act upon critical feedback.
Peer code reviews do take time, and may be uncomfortable for both the reviewer and reviewee. But in the long run they help us build high-quality products that are easy to extend and maintain. At the same time they facilitate code reuse, meaning a big win in terms of efficiency. To read in depth here is the link of the book Clean Code In C# by Jason Alls.
Further reading https://docs.microsoft.com/en-us/visualstudio/code-quality/?view=vs-2019: This documentation by Microsoft provides information on the different tools available to help you analyze and improve the quality and maintainability of your code. https://en.wikipedia.org/wiki/Code_review: There are many useful links on this page to further your knowledge of code reviews and their value to your business. Gang-of-Four design patterns book: https://springframework.guru/gang-of-four-design-patterns/ .NET Design Patterns, by Praseed Pai and Shine Xavier: https://www.packtpub.com/application-development/net-design-patterns GitHub’s help page: https://help.github.com/en