Open-Source Code Quality - ChoETL

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.

 

ChoETL	Code Quality Rating
https://github.com/Cinchoo/ChoETL	8.08

 

Cinchoo ETL is a .NET library for reading and writing CSV, Fixed Length, XML, and JSON files. It supports reading and writing of custom POCO class objects. I am currently using ChoETL in a project where via ASP.NET, we allow customers to download and upload CSV files. Here are the results of my code analysis. I would like to note that I could not calculate unit test coverage since 157 tests are broken. The code quality rating shows there is a code violation every 8.08 lines of code.

 

Errors	Warnings	Information	Unit Test Coverage
0	65,074	14,568	UNKNOWN – Broken
Maintainability Index	Cyclomatic Complexity	Depth of Inheritance	Class Coupling
88	147,107	2,229	70,464
Lines of Source Code	Lines of Executable Code	Lines of Cloned Code	Code Commenting
643,902	231,796	22,190	Grade of F

 

Coding Standard Issues

 

Here are just some examples of what needs to be fixed from the near 80K violations.

 

Using string.Empty

 

Here is a common issue I see in most code bases I analyze,

ele.Add(new XAttribute(kvp.Key.Replace("@", ""), value));

The issue here is that they are using ""which can affect performance. Instead usestring.Empty. Here is how it should look,

ele.Add(new XAttribute(kvp.Key.Replace("@", string.Empty), value));

Variable Naming

 

Here is an issue of local variable names.

ChoFallbackValueAttributeFallbackValueAttribute = (from a in mi.Attributes.AsTypedEnumerable<Attribute>() where typeof(ChoFallbackValueAttribute).IsAssignableFrom(a.GetType())select a).FirstOrDefault() as ChoFallbackValueAttribute;

The issue is that local variable names should start with lower-case letters. Here is how I would fix it.

var fallbackValueAttribute = (from a in mi.Attributes.AsTypedEnumerable<Attribute>() where typeof(ChoFallbackValueAttribute).IsAssignableFrom(a.GetType()) select a).FirstOrDefault() as ChoFallbackValueAttribute;

Missing Accessibility Modifiers

 

All properties, methods, fields, events, etc. should have proper accessibility modifiers. Here is an example of the violation.

ChoYamlRecordFieldConfigurationfieldConfig = null;

It should be coded like this (includes proper naming).

private ChoYamlRecordFieldConfiguration _fieldConfig;

Braces Placement

 

Another common issue I see in code is the improper use of braces, usually not using them at all like in this example.

if (methodInfo != null)
    return methodInfo.Invoke(target, args);
else
    throw new ApplicationException(String.Format("Can't find {0} method in {1} type.", name, target.GetType().FullName));

Here is how it should have been coded.

if (methodInfo != null)
{
    return methodInfo.Invoke(target, args);
}
else
{
    throw new ApplicationException(string.Format("Can't find {0} method in {1} type.", name, target.GetType().FullName));
}

The other issue with this code is the use of ApplicationException. Developers should never throw this exception since it is reserved for .NET. They should have used a different one or created a custom one inheriting Exception.

 

Proper braces should be used for foreach statements. Here is an example of the violation.

foreach (object rec1 in buffer)
    yield return new ChoDynamicObject(MigrateToNewSchema(rec1 as IDictionary<string, object>, recFieldTypes));

Here is how to fix this.

foreach (var record in buffer)
{
    yield return new ChoDynamicObject(this.MigrateToNewSchema(record as IDictionary<string, object>, recFieldTypes));
}

This foreach could improve its performance by changing it to use for as shown in this example.

for (var recordCount = 0; recordCount<buffer.Count; recordCount++)
{
    yield return new ChoDynamicObject(this.MigrateToNewSchema((IDictionary<string, object>)buffer[recordCount] as IDictionary<string, object>, recFieldTypes));
}

Prefix Local Calls

 

When using local calls, they should be prefixed with either this or base. Here is the violation.

public Type RecordMapType
{
    get { return _recordMapType == null ? RecordType : _recordMapType; }
    set { _recordMapType = value; }
}

Here is how to fix the issue.

public Type RecordMapType
{
    get
    {
        return this._recordMapType == null ? this.RecordType : this._recordMapType;
    }
    set
    {
    this._recordMapType = value;
    }
}

Remove Dead Code

 

Before committing code to main branches, make sure to remove any code that is not being used anymore. Part of this is to remove any code that is commented out, which this codebase has a lot. Also, remove any classes, class members, and parameters that are not being used. For example, the code below has an unused parameter.

private object DeserializeNode(object yamlNode, Type type, ChoYamlRecordFieldConfiguration config)
{
    if (_fieldConfig.ItemConverter != null)
    return RaiseItemConverter(config, yamlNode);
    else
    return yamlNode;
}

The type parameters are not’ used in this code. Here is how I would fix it.

private object DeserializeNode(object yamlNode, ChoYamlRecordFieldConfiguration config) {
    return this._fieldConfig.ItemConverter != null ? this.RaiseItemConverter(config, yamlNode) : yamlNode;
}

Don’t Set Variables to Their Default State

 

Again, I see many developers setting variables to their default state. The biggest issue is that it can cause performance issues. Here is an example.

string colName = null;
Type colType = null;
int startIndex = 0;
int fieldLength = 0;

To fix, just remove the assignments like in this example.

string colName;
Type colType;
int startIndex;
int fieldLength;

Use Object Initialization

 

To properly initialize property values in an object, use object initialization. Here is an example of code not using this.

var obj = new ChoXmlRecordFieldConfiguration(fn, xPath: $"./{fn1}");
obj.FieldName = fn1;
obj.IsXmlAttribute = true;

This is how you would use object initialization.

var obj = new ChoXmlRecordFieldConfiguration(fieldName, xPath: $"./{fieldName1}")
{
   FieldName = fieldName1,
   IsXmlAttribute = true
};

Other issues include class members are not ordered correctly. Very little code documentation which means IntelliSense does not work for any of the methods. Many of the projects have references that are not being used anymore. It uses obsolete methods. Classes should be in separate files and more.

 

Summary

 

While there are not any errors in this codebase (except for unit tests), the biggest issues are,

1. IntelliSense is missing for all the methods and classes. This makes it much harder to use. I had to spend too much time trying to figure things out.
2. Poor or inconsistent coding standards produce “spaghetti” code.
3. It needs work to improve performance.
4. It needs better examples on how to use it online. For example, I could not find any examples of how to import a CSV from the ASP.NET file stream. All examples showed getting the file from a saved location, something that you should always try to avoid in ASP.NET due to IO performance issues.
5. There are over 22K lines of potential duplicate code… a maintenance nightmare!

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

 

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.