Analyzing Not So Super Code

Not “Super” Code with Over 7,000 Violations

I have been analyzing code for issues for a very long time. In this article, I will share what I found at a recent contract I was working on. I have analyzed some bad solutions in my career, and this is hands down one of the worse I have done in recent memory. I will show you just some of the over 7K violations. The biggest issue with this solution, which is in production, is the very poor architecture it has. Honestly, I do not know if there was any architecture for these 7 projects.

This reminds me of what the developers asked me during the interview process at another company.

The Numbers

Let us start with the numbers I was able to get before I show you examples of code violations. This codebase has so many issues that Analyze in Visual Studio and the add-in CodeIt.Right would freeze. I had to use Task Manager to kill Visual Studio. The Code Clones tool in Visual Studio would not work either. The only way I could get the numbers below is to remove one of their projects, which includes 7 proxy classes created from WSDL with over 120K lines of code each! So, these numbers are not accurate, but I think you will get the picture.

Cyclomatic Complexity

This solution has a total cyclomatic complexity of 36,831! I have spoken and written about what this number means to me. To properly unit test encapsulation, this solution needs a minimum of 36,831 unit tests, but it has only 47! I am not sure if they even work since the people on my team that worked on the solution tell me that the only way to test code is to do it in production. Yes, you heard right! This is how they test changes.

The Other Numbers

The low depth of inheritance shows that there is very little code reuse. I did not see much code reuse in the project, and I saw a lot of duplicate code. This is a maintenance nightmare. The high number for class coupling shows that making changes to one class might cause issues in other classes. This number should be low.

Also, this solution has over 18,000 spelling issues! On top of that, there are only 35 places in the code that deals with globalization/ localization. This will be very costly for the company to fix in the future if it needs to support a different spoken language especially since they do not use resource files for string at all.

Coding Violations

Okay, let us get to the code violations. With over 7K issues, I have picked code to give you a flavor of what they are. Some, I have never seen before.

Comparing Strings

Using equals (==) to compare strings is not recommended for many reasons including performance and localization.

if (headers[i] == "Order ID")
{
  id_index = i;
}

Not only is this improper, but “Order ID” should be a constant.

if (string.Compare(headers[headerCount], OrderId, StringComparison.Ordinal) == 0)
{
  idIndex = i;
}

As shown in my book about code and app performance, using StringComparison.Ordinal can improve performance.

String Interpolation

String interpolation is newer to .NET and in many cases, it can make the code more performant. Here is an example of the violation.

log.DebugFormat("Save Order Information : " +item.Count);

Here is how to fix it using string interpolation.

log.DebugFormat($"Save Order Information : {item.Count}");

When should you use string interpolation? My recommendation is to use it on any string that is NOT displayed to the user since so far it does not support globalization/ localization. Anything displayed to the user should use String.Format with CultureInfo.

Checking String for Null

I still see checking for a null or empty string improperly in much of the code that I analyze. Here is an example of the violation.

if (ID != null && ID.Length > 0)
{
  address.ID = new ID();
  addrInfo.ID.Value = ID;
}

Here is how to properly check for null or empty.

if(String.IsNullOrEmpty(id) == false)
{
  address.ID = new ID();
  addrInfo.ID.Value = id;
}

This is more performant!

Simplifying Collection Initialization

You should use the simple way of initialization of a collection when possible. This code shows the violation.

var addresses = new List<Customer.Address>();
addresses.Add(address);

Here is how to fix it that is more readable.

var addresses = new List<Customer.Address>
{
  address
};

Pattern Matching

Using pattern matching can also speed up the code. Here is an example of the violation.

if (entry.Entity is Order)
{
  Id = ((Order)entry.Entity).ID;
}

Here is how to properly use pattern matching.

if (entry.Entity is Order order) 
{
  id = order.ID;
}

Variable Assigned Twice

I found that throughout their code they are assigning variables twice! This is a performance issue since it causes more instructions to be created. Here is an example of the violation.

IList<string> ids = null;          
ids = OrderItemExists(order);

This should just be one line like this.

var ids = OrderLineItemExists(order);

Not Async at All!

I found over 130 async/ await issues like the code below. After looking at the issues, it is clear to me that this team does not understand how async/await works. Using this improperly can cause a lot of issues.

At one company I worked at then they implemented multi-threading (before we had async/ await) to speed up the code, it made the application so slow it took them months to figure out how to fix it. My advice is, if you do not understand multi-threading, do not use it until you do! Then make sure you do a lot of benchmark tests.

Here is an example that was duplicated many, many times throughout the code.

public Task<service.Response> LogInAsync(Service.Request Input)
{
  return base.Channel.LogInAsync(Input);
}

As you can see, they are not using async in the method definition and they are not using await when calling LogInAsync(). This how they use this method in their code.

var result = await service.LogInAsync(new Session.LogInRequest() { Username = App_User, Password = App_Password });

They are using await, on a method that does not even implement it! Here is how to fix this mess.

public async Task<service.LogInResponse> LogInAsync(Service.LogInRequest input)
{
  return await base.Channel.LogInAsync(input);
}

Calling the code now properly uses ConfigureAwait().

var result = await service.LogInAsync(new Session.LogInRequest() { 
  Username = App_User, Password = App_Password }).ConfigureAwait(false);

Many IDisposable Issues

There are a lot of IDisposable issues in the code. I have written about this many, many times and this can cause virtual memory leads that can eat up all the memory on the server. I have seen this at a lot of companies that hire me to fix code issues.

At one company, during a meeting, after I got a look at their code, I asked the manager “Do you have any servers you have to reboot every once in a while, and you don’t know why?” He said “YES!” Then I said, “I know exactly why!”

Here is an example that has many of these violations.

RijndaelManaged symmetricKey = new RijndaelManaged();
symmetricKey.Mode = CipherMode.CBC;
ICryptoTransform decryptor = symmetricKey.CreateDecryptor(keyBytes, vectorBytes);
MemoryStream memoryStream = new MemoryStream(textBytes);
CryptoStream cryptoStream = new CryptoStream(memoryStream, decryptor, CryptoStreamMode.Read);
byte[] textBytesArray = new byte[textBytes.Length];
int byteCount = cryptoStream.Read(textBytes, 0, textBytes.Length);
memoryStream.Close();
cryptoStream.Close();
return Encoding.UTF8.GetString(textBytes, 0, byteCount);

Here is how to properly use the types above that implement IDisposable.

using (var symmetricKey = new RijndaelManaged())
{
  symmetricKey.Mode = CipherMode.CBC;

  using (ICryptoTransform decryptor = symmetricKey.CreateDecryptor(keyBytes, vectorBytes))
  {
    using (var memoryStream = new MemoryStream(textBytes))
	 {
      using (var cryptoStream = new CryptoStream(memoryStream, decryptor, CryptoStreamMode.Read))
      {
        var textByteCollection = new byte[textBytes.Length];
			var byteCount = cryptoStream.Read(textBytes, 0, textBytes.Length);
        return Encoding.UTF8.GetString(textBytes, 0, byteCount);
      }
    }
  }
}

I would like to point out is that I am not closing the streams since that will be done when Dispose is called at the end of the code block. I have yet to find a tool, including Visual Studio that correctly finds these types of issues. It has always been a very manual process.

Different Environments Too Hard to Setup

Most of the data for this solution comes from Salesforce. I have never liked using Salesforce. It is slow, bloated and in this case, the service references that were built using their WSDL was a nightmare. These huge services references cause Visual Studio to crash throughout the day. This was made worse by the company forcing the User folder onto a network drive. This is the #1 reason, I refused to use their laptops, so I used my own. Most of the time, in my team, I was the only one who could even build it! Crazy!

To use the different Salesforce environment, the team chose to put the following at the top of most of their classes to deal with it. As you can see, it is messy.

#if PRODUCTION
  using SF = SlowApp.SFServices.salesforce;
#elif UAT1
  using SF = SlowApp.SFServices.salesforcebackup;
#elif UAT2
  using SF = SlowApp.SFServices.salesforece2;
#elif TEST
  using SF = SlowApp.SFServices.salesforcetest;
#elif DEV1
  using SF = SlowApp.SFServices.salesforcedebug;
#elif Dev2
  using SF = SlowApp.SFServices.salesforce4;
#endif

I have been using .NET for 21 years and I never seen something like this implemented like this! When I had to use different Salesforce environments in the project that I architected, I used different NuGet packages and put it in the project file. Here is how I did it.

<ItemGroup>
  <Reference Include="FastApp.Services.Salesforce" Condition="'$(Configuration)'=='Debug'">
    <HintPath>..\FastApp.Services.Salesforce \bin\Debug\netcoreapp3.1\FastApp.Services.Salesforce.dll</HintPath>
  </Reference>
  <Reference Include="FastApp.Services.Salesforce" Condition="'$(Configuration)'=='Release'">
	<HintPath>..\FastApp.Services.Salesforce \bin\Release\netcoreapp3.1\ FastApp.Services.Salesforce.dll</HintPath>
  </Reference>
  <Reference Include="FastApp.Services.Salesforce" Condition="'$(Configuration)'=='Staging'">
    <HintPath>..\FastApp.Services.Salesforce \bin\Staging\netcoreapp3.1\ FastApp.Services.Salesforce.dll</HintPath>
  </Reference>
</ItemGroup>

Doing this using NuGet packages is so much easier and cleaner and in only one place! Just think how hard it would be to modify their way of doing it throughout 100’s classes! That is why the company said that I ran the most successful project in the company’s history! Then they canceled my contract.

What the heck is this??

Never have I ever seen this before! Tell me what is missing?

#elif Dev2
using SF = SlowApp.SFCServices.sfdcindia;
#endif

If you guessed the #if, you are correct!

Architecture, The Biggest Issue!

The lack of any coherent architecture is the biggest issue with this solution! It is the worse architected solution that I have seen in my recent memory. The six main projects in this solution should have been many more if proper architecture, including code reuse, was done. Because of this, there were so many issues! When the company I worked for purchased the company that wrote the original version of this code I guess they knew it needed fixing since they renamed all the projects and put the word “Super” in front of it. As you can see, this code is very far from super!! The code is very brittle and very hard to fix, add new features and make it impossible to unit test. If they just used common patterns like SOLID or DDD, this solution would have been so much better. Unfortunately, I did not see any patterns used in this codebase.

The code is hard to understand since nothing in it came even close to common .NET coding practices. Towards the end of my contract this year, my manager asked if I would work on this solution. I guess he was trying to find a way to keep me. Even though I knew that it might be canceled, I told him I would not work on it. I followed it up with if they want me to re-architect this mess, I would be happy too. That did not happen.

Other issues

Here are just some of the other issues I found.

• Lots of code that is not being used. Ranging from parameters, classes, private members, and more!
• Very few XML comments!
• No code analysis was configured for any of the projects which are very apparent by just looking at their spaghetti code!
• No OOP encapsulation validation!
• In-line SQL! This can lead to SQL injection attacks.
• A lot of duplicate code!
• Fields that should be private, are not.
• Code not formatted correctly.
• Defining the same namespace twice in classes.
• Improper use of string interpolation.
• File names do not match the type name.
• Multiple classes in a file.
• Unused private members and private members not being assigned.
• Improper use or missing braces in C#.
• Code from the repository does not build without a lot of changes that are not documented.
• Missing accessibility modifiers.
• Code looks like it has been written by different developers. Using and enforcing coding standards could fix this, but it is clear they do not. Even the same class has different naming standards!

Summary

I expect much better code from a well-known company in the United States... well I expect this at every company I work at! I will admit that I have high standards. When I first started, I tried to make changes to the code, including switching all the huge proxy classes into NuGet packages. They did not accept any of my pull requests. That was a waste of over a month.

One reason I worked on fixing their issues is to show them the standard way that C# software engineers write code. Unfortunately, this fell on deaf ears. The lead developer, who recently quit, thought that he knew the best way to write code and it was fine. I made up a term for these types of programmers. I call them Progniant, which means they will not listen to anyone and thinks they know everything. I call myself a Simplifier. I write code in a way that almost anyone can understand.

Even the new lead for that team had someone in my team change how he wrote LINQ since the lead said his way was more performant. He did no testing to back up his claims. So, I benched marked what he was telling my teammate to do, and you guessed it, it is slower! I also told the people on my team to never use this code as a learning experience, since they would learn in the incorrect way of coding.

To learn the proper way of implementing coding standards for .NET, please pick up a copy of my coding standards book on Amazon http://bit.ly/RockYourCodeBooks!

If your team needs a code review of their solutions, you can always reach out and hire me! https://bit.ly/ContactDotNetDave