Open-Source Code Quality - Foundatio

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. How does your open-source 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.

 

Foundatio	Code Quality Rating
https://github.com/FoundatioFx/Foundatio	15.42

 

Foundatio is a pluggable foundation blocks for building loosely coupled distributed apps which includes implementations in Redis, Azure, AWS, RabbitMQ, and in memory (for development). I have not used these projects, but it looks very interesting. The code quality rating shows there is a code violation every 15.42 lines of code, which is the best rating I have seen since starting these articles.

 

Errors	Warnings	Information	Unit Test Coverage
919	1,232	3,520	249 – Broken
Maintainability Index	Cyclomatic Complexity	Depth of Inheritance	Class Coupling
89	20,675	673	18,538
Lines of Source Code	Lines of Executable Code	Lines of Cloned Code	Code Commenting
87,429	37,212	717	Grade of D

 

Coding Standard Issues

 

Here are just some examples of what needs to be fixed from the 5,671 violations in this solution.

 

Using ConfigureAwait

 

All the 919 errors above were this issue: CA2007 - Consider calling ConfigureAwait on the awaited task. Here is an example of the code that violates this rule,

await storage.SaveFileAsync(path, memoryStream);

This is fixed by adding ConfigureAwait() as recommended by Microsoft for DLL’s,

await storage.SaveFileAsync(path, memoryStream).ConfigureAwait(continueOnCapturedContext: false);

Braces Around If Statements

 

One of the issues I see a lot is not properly adding braces to if statements. It is my feeling that this is due to software engineers coming from other languages like Java. Here is an example,

if (_logger.IsEnabled(LogLevel.Trace)) _logger.LogTrace("Setting Local cache key: {Key} with expiration: {Expiration}", key, expiration);

Here is how to fix this violation,

if (this._logger.IsEnabled(LogLevel.Trace))
{
   this._logger.LogTrace("Setting Local cache key: {Key} with expiration: {Expiration}", key, expiration);
}

This library also does not use proper braces placement for else, foreach, lock, using, and while statements. There is a lot of cleanup work that should be done to fix these violations.

 

Accessibility Modifiers

 

Something very important for object-oriented programming is to property set up classes for their intended use. Every class, property, and method should have a specific accessibility modifier specified. This is very important. Here is an example of the violation,

class RegisteringMemoryStream : MemoryStream, IDisposable

It should have been defined like this,

private class RegisteringMemoryStream : MemoryStream, IDisposable

Here is an example of an issue with a field,

readonly IMessageSink diagnosticMessageSink;

It should have been defined like this,

private readonly IMessageSink diagnosticMessageSink;

Pattern Matching

 

Pattern matching is a way to speeding up your code. Here is an example of the violation,

if (!(metrics is IMetricsClientStats stats))
return;

This violation can be fixed like this,

if (metrics is not IMetricsClientStats stats)
{
   return;
}

Inlining Out Variables

 

Something newer to .NET is the way we create variables for out parameters. The older way of doing this is shown below,

object value;
if (_structAsObjectCache.TryGetValue(type, out value)) return value;
value = adder(type);

Here is how to fix this violation,

if (_structAsObjectCache.TryGetValue(type, out var value))
{
   return value;
}

I have never been a big fan of out parameters, but at least doing the variables inline makes the code a bit cleaner.

 

Other Common Violations

 

Here are some other common issues I see in this code base. I have previously written about all of these issues either in my coding standards book or online.

1. Multiple classes in the same file.
2. Unnecessary assignment of values.
3. Classes are not organized as per StyleCop rules.
4. Missing file header documentation.
5. Much of the code is missing (XML) documentation.
6. The implementation of IDisposable is not implemented properly in classes.
7. Fields that should be read-only are not properly defined.
8. I could not find any methods that properly validated the parameters. This is so important for proper object-oriented programming. This means everything I analyzed breaks encapsulation. The codebase has only 249 unit tests. That number needs to be over 2,500 just to properly test encapsulation!!

Summary

 

Now that I have analyzed these projects, I think I will wait until they fix all the errors and warnings and the IDisposable issues. Therefore, it is so important to analyze projects before just adding them to your projects.

 

To learn the proper way of implementing coding standards for .NET, please pick up a copy of my coding standards book on Amazon!

 

Go here to view the .editorConfig that I use to analyze projects.

 

I hope that the next time you want to download a NuGet 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? Please comment below.