Best Practices For Code Review

This article provides a high-level overview of the code review process for C# code, as well as best practices for code review.

Code review is an essential part of any developer’s job. Code review is a technique that allows another developer (who is not necessarily on the same team or working on the same feature within the same team) to go over your code line by line and identify areas for improvement or point out holes. As a result, code review is a process rather than a technology. However, there are some developer productivity tools (discussed later in this article) that can help a developer write high-quality code.

Is it a must to have my code reviewed by other developers?

You absolutely must. However, it is dependent on your team’s development methodology and ALM (Application Lifecycle Management) tool configuration. There are ways to avoid code review by simply not doing it and checking in the code. However, it is not advised.

What if my code is not reviewed?

It’s difficult to say, but at times and in some cases, it may cause serious problems and raise concerns for the entire team. For example, a condition check is missed, null is not handled properly, or your exception handling block is not handling all error situations. These examples may appear to be very simple, and most developers do write defensive code that addresses such issues. However, code review can also be beneficial to your “design,” “techniques,” “patterns,” “approach,” and so on.

How to find a code reviewer?

I’ve worked in a variety of team environments and work environments. Many times, I’ve been asked to perform a code review for a developer who isn’t on my team, or rather, isn’t on their team. But what I admire and appreciate here is that someone reached out to another developer in case their team’s developer was unavailable or was preoccupied with critical production issues, etc. This mindset allows the team to deliver the best written code possible.

Let’s Begin with Best Practices of Code Review

I’ll go over some of the most important aspects of code review in this section. I attempted to highlight the fundamental best practices that are required in any code review. I’ve seen many times in code reviews where developers are more focused on looking for code design patterns or specific areas in code review.

Project and File names

Many developers only focus on code, but in my opinion, your project name, file name, and so on are also important. A well-defined project and file name is equally important and adds a lot of clarity by offering a sense of purpose being served by a solution, project, or file.

Always begin from the top

Before you begin scanning the code blocks/statements in a class (.cs) file, I recommend that you first examine the “using” statements in any .cs file. I’ve seen developers add numerous “using” statements while experimenting with different code blocks to achieve functionality. However, once that functionality is complete, many developers fail to clean up references to those “using” statements that are no longer required and must be removed.

Visual Studio shows unused using statements in gray color which can be safely removed as shown in the image below.

Sort all the using statements

The standard development practice is to keep adding new using statements at the end of previous ones, as shown below.

using System;
using System.Collections.Generic;
using System.Collections;
using Newtonsoft.Json;
using System.Threading.Tasks;
using System.Linq;

Assume that code consumes all of the new using statements, but the problem is that they are not sorted. Sorting all using statements is one of the best coding practices. To sort using statements go to Edit -> IntlliSense -> Sort Usings

Following the “Sort Usings” operation, all of the using statements will be alphabetically sorted (this is the only order A Z) and arranged as shown below.

using Newtonsoft.Json;
using System;
using System.Collections;
using System.Collections.Generic;
using System.Linq;
using System.Threading.Tasks;

Don’t just ignore warnings

As developers, we are more concerned with compilation errors and the success of the project/solution. Consider the code below, for example.

namespace CodeReview
{
	class Program
    {
    	static void Main(string[] args)
        {
        	int i,j;
            i = 10;
            Console.Write(i);
        }
    }
}

This is very simple code, and the project will build without any compilation errors. However, as you can see, “j” is not used anywhere in the code, so this will result in a warning, which many developers don’t seem to mind.

In my opinion, and in the opinion of many software companies, including many Microsoft teams with whom I have worked, a 0 warnings policy should be enforced before check-in or commit. As a result, it is critical to examine the “Error List” View menu Error List, and then observe the Warning Tab, which must also show “0 Warning” just as we always work to see “0 Errors.”

Code Consistency

“Consistency” is a critical quality of well-written code. That is, stick to one style. Assume you want to declare an integer variable, and it goes without saying that different teams and developers will have different coding standards. Many times, developers disregard this, and the code contains scattered instances of the types/keywords Int32 or int and String or string, indicating that code consistency is violated.

Using mixed statements, on the other hand, will successfully compile the code but will result in a completely inconsistent code base.

namespace CodeReview
{
	class Program
    {
    	static void Main(string[] args)
        {
        	int i = 10;
            Int32 j = 5; // Be consistent using either int or Int32
            Console.Write(i);
        }
    }
}

Do care for Null all the times

Null has a disastrous effect on the functionality of your code. If you ignore a null check, you will suffer the consequences. This is why many developers productivity tools, such as Re-Sharper, alert you to any potential “NullReferenceException” in your code.

Make sure that all of your if/else statements account for nulls and have guard(s) in place; most of the time, IsNullOrEmpty or IsNullOrWhiteSpace will come in handy.

Dead code

Many times, I’ve observed that as developers, we are primarily concerned with our own code; specifically, code that we write. We occasionally see a block of code that has been commented for a long time, but we don’t seem to mind, and why should we? There are two reasons for this, in my opinion. First, I don’t care as long as my code works. Second, I work in an agile team and am committed to completing my tasks within the timeframes I have set.

The first is an attitude issue, and the second is a lack of time to complete the work. But one thing is certain: both can do it. Open a ticket in the Technical Debt backlog with the details for that file to be cleaned up.

Naming Convention

Today’s developers are well aware of the differences between Camel and Pascal casing and when to use one over the other. Variable and parameter names, for example, must use Camel casing, whereas class, function, and method names must use Pascal casing. Make certain that this fundamental principle is followed consistently.

Code Readability

Many times, code is written in such a way that no one can make sense of it. That is, all of the code is jumbled up and one line after the other, making it barely readable. Check that the code has proper single line spacing between code blocks wherever applicable.

Handling Unmanaged Resources

Even though the .NET Framework takes care of memory management through GC (garbage collection), there are limitations on garbage items and where they can be collected. Cleaning up expensive resources like File I/O handles, connections, Network resources, and so on is often a good idea.

If you want to automatically handle the disposal of objects once they are out of scope, you can use the “using” block for unmanaged code to dispose of such items once their usage is over. Finally block is another good place to handle such code, for example, File I/O operation to read a file.

Write Defensive Code

.NET Exception Handling is essential for writing defensive code and protecting your application from runtime anomalies. Try/catch and finally are three magic words that will solve the majority of your problems.

In terms of application monitoring and troubleshooting, proper implementation and use of Exception Handling, as well as logging any exception messages and details, can add a lot of value to the application.

Declare access specifiers explicitly

C# .NET defines default scope for various types; for example, a variable is private by default, so we like to write “int i in a class, but it’s more appropriate to be declarative and write “private int i.”

Self Documenting Code

Many times, developers adhere to naming conventions (camel or pascal, for example), but the names assigned to variables, methods, functions, and classes are completely meaningless. As a result, developers must sometimes write lengthy code comments to explain the purpose of each function or variable, for example.
Giving “self-describing” names, in my opinion, will add a lot of value while also saving developers’ time in documenting the code; as many software teams have a practice of writing a comment above each line of code to explain the purpose of the code below.

For example, instead of “int age;” you could declare “int studentAge;”. Similarly, rather than simply writing a function called “GetData(),” it is preferable and more helpful to say “GetStudentsReportData().” Class names, functions, and Unit Tests, for example, can all benefit from similar techniques. However, in the case of unit tests, the name(s) may become quite lengthy, which is perfectly fine and acceptable.
As a result, naming a unit test method “TestSuccessWithOneStudentRecordAddedToDb” is preferable to simply saying “TestOneRecordData.”