Best Practices For Code Review

Abstract

This article provides a broad overview of the review process for the code written in C# using Visual Studio 2015 and also uncovers best practices for code review.

Code Review is a very important part of any developer’s life. Code review is a technique which allows another developer (not necessarily working in same team or same feature within a team) to go over-n-through your code line-by-line and identify areas for improvement or point out holes. Hence, code review is a process and not a technology. However, there are some developer productivity tools available (mentioned later in this article) which can enable a developer to write good quality code.

Disclaimer

There are numerous guidelines and best practices software development teams follow and depend on. This article will uncover most of those but it may not cover all the best practices around code review. However, I want to assure you that all the guidelines mentioned below will help any individual to practice and apply good coding principles and perform a quality code review.

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

Well, you must. But it depends on your team’s development methodology and ALM (Application Lifecycle Management) tool settings. There are ways to bypass the code review by simply just not doing it and straight checking-in the code. It’s not recommended though.

What if my code is not reviewed?

It’s hard to say, but at times and in some cases it might cause serious issues and raise concerns for the whole team. For instance, a condition check is missed, or null is not handled properly or not all error situations are being handled by your exception handling block. Well, these examples might sound very simple and most of the developers do write defensive code and cover such aspects. But what code review can also provide value towards is your “design”, “techniques”, “patterns”, “approach” etc.

How to find a code reviewer

I have worked in various team settings and work environments. Many times I have been invited to perform a code review for a developer who is not part of my team, or rather, I am not part of their team. But what I admire and appreciate here is the fact that someone reached out to another dev in case their team’s developer is either not available or busy with critical production issues, etc. Such a mindset empowers the team to deliver the best written code.

Let’s Begin with Best Practices of Code Review

Here, I am going to list out some areas which are critical from a code review perspective. I tried to highlight the core best practices which are a must in any code review. I have seen many times that in a code review developers are more focused to look for code design patterns or some areas in code review,

1. Project and File names
   
   Many times a developer’s only focus on code, but in my view your Project name, file name etc. also matters a lot. It’s not sufficient to deliver well written functional code; a well-defined project and file name is equally important and brings a lot of clarity by offering a sense of purpose being served by a solution, project or a file. For instance see the solution and project name as shown in the image below.
   
   
   Figure 1 - Properly named Solution and Project
   
   
2. Always begin from the top
   
   Before you really start scanning through the code blocks/statements in a class (.cs) file; I would recommend that you must 1st look at the “using” statements in any .cs file. I have personally observed that many times developers add numerous “using” statements while trying various code blocks to achieve the functionality. But once that functionality is accomplished; many developers tend to forget about cleaning up references of those “using” statements which are no longer needed now and must be removed.
   
   Visual Studio 2015 shows unused using statements in gray color which can be safely removed as shown in the image below.
   
   
   Figure 2 - Un-used using statements are greyed out by default in Visual Studio 2015
   
   
3. Sort all the using statements
   
   Usual development practice is that we keep adding new using statements at the end of previous ones, for instance as shown in the image below.
   
   
            Figure 3 - Newly added using statements
   
   Let’s assume that code is consuming all of the added using statements, but the issue is that these are not sorted. One of the coding best practices is to Sort all using statements. To sort using statements right-click in code editor windows and click on “Organize Usings” then click on “Sort Usings”.
   
   
                           Figure 4 - Sort Usings option in Visual Studio 2015
   
   After performing the “Sort Usings” all the using statements will be alphabetically sorted (this is the only order A Z) and will be arranged as shown in the image below.
   
   
   Figure 5 - Alphabetically Sorted Using statement.
   
   
4. Don’t just ignore warnings
   
   As a developer we are more focused on compilation errors and want to see that project/solution build successfully. For instance, consider the code as shown in the image below,
   
   
                  Figure 6 - sample code
   
   This is a very simple code and project will build successfully without any compilation errors. However, as you can see that “j” is used nowhere in the code and so this will cause a warning; which many developers doesn’t seem to care about.
   
   In my view and as many software companies including many Microsoft teams I have worked with enforce 0 warnings policy before check-in. Hence, it is very important to look in detail at the “Error List” View menu  Error List, and then observe Warning Tab and must try to have that also show “0 Warning” just like we always work to only see “0 Errors”.
   
   
                     Figure 7 - Error List, highlighting warning in the code
   
   
5. Code Consistency
   
   One very important quality of well written code is “Consistency”. I.e. stick to one style. Let's say that you want to declare an integer variable and it’s obvious that different teams and developers will have different coding guidelines. Many times developers ignore this and code has scattered instances of types/keywords Int32 or int and String or string, this demonstrates that code consistency is violated.
   
   However, using mixed statements will successfully compile the code but makes your code base completely in-consistent.
   
   
         Figure 8 - Code consistency violation
   
   
6. Do care for Null all the times
   
   Null has catastrophic impact on your code functionality. Simplyignore a null check  and you will face the consequences. This is why many developer productivity tools like Re-Sharper prompt for any potential “NullReferenceException” which can be triggered from your code.
   
   Make sure that all your if/else take enough care of null and have guard(s) in place, most of the time IsNullOrEmpty or IsNullOrWhiteSpace will come really handy.
   
   
7. Dead code
   
   Many times I have seen that as a developer we are mostly interested in our own code; to be specific, code we are writing. At times we do observe a block of code which is commented for long, but we don’t seem to care, but why not? Well, in my view there are two reasons for this. First, I don’t care as long as my code works. Second, I am working in an agile team and I am committed to finish my task in committed timelines.
   
   First one is an attitude problem, second is timeline and hours remaining to complete the work. But both can do one thing for sure. Open an item in Technical Debt backlog with details to have that file cleaned up.
   
   
8. Naming Convention
   
   All developers today are well aware of Camel and Pascal casing and when to use one over the other. For instance variable and parameter names must follow Camel casing and for class, function, and method names Pascal casing must be used. Make sure this basic principle is applied consistently.
   
   
9. Code Readability
   
   Many times code is written in a way that no one can hardly make sense out of it. I.e. all code is so jumbled up and one line after the other that it’s barely readable. Make sure that code has proper single line spacing wherever applicable between the code blocks.
   
   
10. Handling Unmanaged Resources
    
    Even though .NET Framework takes good care of Memory management via GC (garbage collection) there are limitations on garbage items and from where those can be collected and what not. Many times, it’s wise to handle cleaning of expensive resources like File I/O handles, connections, Network resources, etc. by yourself.
    
    Such items can be disposed of once their usage is over and this can be done using the “using” block for unmanaged code, if you want to automatically handle the disposing of objects once they are out of scope. Another good place to handle such code is finally block for example, File I/O operation to read a file as shown in the image below.
    
    Watch my YouTube channel video here.
    
    
    Figure 9 - Handling unmanaged resources by .NET’s StreamReader
    
    
11. Write Defensive Code
    
    .NET Exception Handling is the key to write defensive code and protect your application against any runtime anomalies. Three magic words to solve most of your problems are try/catch and finally.
    
    Proper implementation and usage of Exception Handling and logging any exception messages and details can add a lot of value to application in terms of application monitoring and troubleshooting.
    
    
12. Declare access specifiers explicitly
    
    C# .NET has default scope defined for various types, for instance a variable is private by default and so in a class we like to write just “int i” but it’s more appropriate to be declarative and instead write it as “private int i”.
    
    Do you know what default scope of a class is in C #? Watch my YouTube channel video here.
    
    
13. Self Documenting Code

    
    
    Many times developers do follow naming convention (camel or pascal etc.) but the given names to variables, methods, functions and class etc. are not meaningful at all. Hence, at times developers write long code comments to describe what is the purpose of each function or variable for instance.
     
    In my view, giving "self-describing" names will add a lot of value and also save developer's time in documenting the code; as many software teams have a practice of writing comment above each line of code to explain the objective of code below.
     
    For instance, instead of "int age;" you may want to declare "int studentAge;". Similarly, instead of just writing a function with name "GetData()" it will be preferred and more helpful to say "GetStudentsReportData()". Similar techniques can also be applied to class names, functions and Unit Tests etc. But in case of unit tests, name(s) might get really lengthy, but this is totally fine and acceptable.
     
    Hence, a unit test method name "TestSuccessWithOneStudentRecordAddedToDb" will be much preferred than just saying "TestOneRecordData".

Summary

Above mentioned code review guidelines are light weight, easy to look for and easy to apply techniques with larger impact on any code base. Mostly it has been evident that simple things are either ignored or not cared about. These best practices can be added up with more guidelines or in combination with other techniques as applicable. Happy Coding…