Is Quality Part Of The Open-Source Projects Your App Is Using?

How important is code quality to your team? It should be at the top of the list, not only to make your customers happy, but make your team happier when bugs arise and when features need to be added. Putting quality in your code in the future is a lot more expensive than doing it when the code is first written.

Since I have been contracting fulltime, I see the number of code violations in a .NET solution from the low thousands all the way to 53,000… in one solution that currently is in production! In that solution, there was a code violation every 4.5 lines of code. Also in that solution, it was difficult to add features and fix bugs. Unfortunately, that company decided to move the code to Java to fix it. They don’t realize that poor code quality is caused by the developer, not the language.

So, if you run FXCop or Visual Studio Analyze on your solution, how many violations do you have? How many are acceptable? My acceptable number is zero, but it really is up to your team. It’s very important not to release the code to quality assurance until it meets the teams’ requirements. During the solution build process, the code should be analyzed and only sent to QA if it meets the teams’ requirements.

So how does your solution compare to some of the most popular GitHub/ NuGet packages? Do you analyze repositories before using them or just adding them via NuGet? If not, you need to keep reading.

Code Quality Rating

To rate the quality of the GitHub projects, I decided to rate the code by counting the number of lines of code and divide that by the number of violations. This way, the higher the number the more lines of code between violations. So, a project that has a rating of 100 has better quality than a project rated 4.5. To determine the number of violations, I used the Microsoft Guidelines (part of FXCop/ Visual Studio Analyze. Before running analytics, I removed any unit test projects.

Unfortunately, some of the projects I wanted to analyze would not load properly in Visual Studio. They were:

• dotnet/corefx
• dotnet/Roslyn

Interesting that they are both Microsoft projects who now owns GitHub.

Newtonsoft.Json	Code Quality Rating
https://github.com/JamesNK/Newtonsoft.Json	95.72

Json.NET is the most popular high-performance JSON framework for .NET and had the best code quality rating of the solutions I analyzed. I use it in many of the solutions I work on at work or my own. Here is the breakdown.

Critical Errors	Errors	Critical Warnings	Warnings	Information
0	75	6	407	3

Here are the top five violations.

1. Source File -> Should have a header (General)
2. Parameter -> Avoid Language Specific Type Names (Naming)
3. Method -> Provide 'default:' for each 'switch' statement (Usage)
4. Interface methods -> Should be callable by child types (Design)
5. Method -> Do not catch general exception types (Exception Handling & Design)

The one I would like to point out here is #5. As I teach in all my code conference sessions, reusable assemblies, should never catch the type Exception. Code in these assemblies, should only catch exceptions that they can deal with, like close a database connection. Otherwise just let the exception bubble up to the calling code. The application layer, an EXE or website, should trap all exceptions to log them and change program flow.

Here is an example from this code base.

As you can see, creating the Version type can throw 5 exceptions. The catch block should only be looking for those 5, otherwise let the exception bubble-up to the calling code. If an exception is thrown that isn’t one of the 5 listed above, then I would contact the project owner, and let them know to get it fixed.

Blazor	Code Quality Rating
https://github.com/aspnet/Blazor	74.99

Blazor is a new .NET web framework using C#/Razor and HTML that runs in the browser with WebAssembly writing in .NET Core. The quality in this framework is very good. One thing I did see is it does not do enough commenting of code and methods. Here is the breakdown.

Critical Errors	Errors	Critical Warnings	Warnings	Information
10	125	0	0	0

Here are the top violations.

1. Field -> Should not have read-only modifier if return type is mutable (Security)
2. Expression -> Specify IFormatProvider (Globalization)
3. Assembly -> Should have strong names (Design)
4. Method -> Do not catch general exception types (Exception Handling & Design)
5. Expression -> Specify StringComparison (Globalization)

I would like to point out #5 which states, there is overload of the System.String.Equals method with System.StringComparison parameter, but it does not. Here is an example.

This issue is an issue due to globalization. When comparing strings, you should always use one of the StringComparison enumerations. Here is how it should be fixed.

As you can see, simply added the StringComparison.InvariantCultureIgnoreCase as the third parameter.

EFCore	Code Quality Rating
https://github.com/aspnet/EntityFrameworkCore	57.64

Entity Framework Core is an object-relational mapper (O/RM) that enables .NET developers to work with a database using .NET objects. It eliminates the need for most of the data-access code that developers usually need to write. Since EF has come out, I’ve always used it for my own projects and encourage most projects at work to do the same.

I don’t have a breakdown of the numbers per severity since the report I use for that number did not work with this codebase. Here are the top 5 issues.

1. Parameter names should match base declaration (Naming)
2. Specify StringComparison (Globalization)
3. Do not declare read only mutable reference types (Security)
4. Property names should not match get methods (Naming)
5. Type, Member -> Should have XML comments (General)

I’d like to point out #3, which I do see in code often. It states, externally visible read-only field has a return type that is mutable. A mutable type is a type whose instance data can be modified. To fix it either the read-only needs to be removed or changed to an immutable type. Here is an example from the project.

This violation was found 172 times in the project. To make it worse, they are exposing a public field. This should NEVER BE DONE! If you do, you are breaking encapsulation, the first pillar of Object Oriented Programming. It seems Microsoft teams needs to use analytics tools more too!

Amazon SDK	Code Quality Rating
https://github.com/aws/aws-sdk-net	30.67

This is the official AWS (Amazon Web Services) SDK for .NET. This was by far the hardest one to analyze and delayed this article by weeks. The solution for the .NET 4.5 version of this SDK contains 147 projects and loading all of them took over 5 minutes. This code base even broke the analyzing tool I used and kept causing Visual Studio to crash. I could count the lines of code and get the total number of violations which is over 32K! I can’t show you the top 5 violations due to the code base breaking the tool I used. I did see the following issue that is common to many code bases that I have analyzed.

Like not catching the base Exception type in reusable assemblies, they have empty catch blocks, as seen below.

They are swallowing the exception which is never good. The try/catch should be removed so the exception will bubble up to the application layer where it can be logged.

Another issue is throwing improper exception types. As seen in the code below they are throwing the type Exception. Throwing Exception should never be done in your code, it’s too generic. Exception is a reserved type for the .NET Framework and should only be used in your code base as a base type for your own exceptions.

I’m working with the NuGet packages for this SDK at the current contract I’m working on. My advice to Amazon; this entire code base needs to be redesigned by a .NET expert! They seem to have written it for Java programmers, not .NET programmers. At this point, .NET does not seem to be a priority for Amazon and this SDK shows it.

It’s difficult to use and the documentation is very limited or doesn’t exist at all. Available code samples are severely lacking too. The last 2 or 3 weeks that I have been using this SDK I feel like I’m the first one that has. There does not seem to be much consistency between the libraries. An example is in some places HTTP status code is a number, in others it’s an enumeration (which it should be everywhere).

Dapper	Code Quality Rating
https://github.com/StackExchange/Dapper	17.48

Dapper is a NuGet library that you can add in to your project that will extend your IDbConnection interface. Below is the breakdown of their results.

Critical Errors	Errors	Critical Warnings	Warnings	Information
4	64	6	50	38

Here are the top 5 violations.

1. Expression -> Specify IFormatProvider (Globalization)
2. Method -> Avoid single line If statement (CodingStyle)
3. Source File -> Should have a header (General)
4. Public Type -> Nested types should not be visible (Design)
5. Assembly -> Should have assembly version (Design)

Out of the top 5, the one that concerns me the most is #1, since it deals with globalization/localization. This means that some of the numbers and strings in their code base might not be converted properly for different languages.

SignalR	Code Quality Rating
https://github.com/aspnet/SignalR	16.51

ASP.NET Core SignalR is a library for ASP.NET Core developers that adds real-time web functionality to your applications. This is a project, from Microsoft and here is the breakdown.

Critical Errors	Errors	Critical Warnings	Warnings	Information
4	100	11	3,147	3,262

Here are the top 5 violations.

1. Type, Member -> Should have XML comments (General)
2. Source File -> Should have a header (General)
3. Interface methods -> Should be callable by child types (Design)
4. Parameter -> Avoid Language Specific Type Names (Naming)
5. Dispose -> Type -> That owns disposable fields should be disposable (Design)

Out of this list, #5 is a very big concern. This is telling me that in at least 51 locations in the SignalR codebase, objects are not being disposed of correctly. This mean that this could cause virtual memory leaks in your application. Here is an example.

In the example above, PerformanceCounter is a disposable type. To fix this, they would need to properly implement the IDisposable interface. This codebase had a total of 114 issues relating to disposing of types correctly.

Summary

How did I analyze these GitHub projects? Well, I have already written the products and workflow that I use in a previous article titled, "My Workflow Before I Submit Code Changes." 

I hope that the next time you want to download a NugGet package for your solution or use an open-source library, you will analyze it first for code quality. I plan to analyze more libraries in the future as a blog post. What library would you like me to analyze?

Drop here!