In Part 1 in this series of articles about real world coding issues, I discussed violations that I found in the code base for a contract I worked on. I showed the statistics for this solution that has over 125K of code violations. Other issues I discussed were cyclomatic complexity, memory issues, performance improvements and more. If you haven’t already, please go here to read part 1: https://www.c-sharpcorner.com/article/real-world-coding-issues-part-i/
For this article, I will show common coding issues that I see in most of the projects I work on that are in the "style" category of the code analyzers. Even though these are under this category, many also affect code performance, something that I specialize in.
Remove Unnecessary Casts
Type casting converts one type to another. Unnecessary type casting not only makes the code less "clean", but it also can be a performance issue. This code shows the issue:
return new CSDateTime(csDate.AddMinutes((double)units), this.DateType);
In this code example, csDate is a DateTime and units is an integer. In this case, an integer can be easily used in a parameter type that is a double. To fix it, just remove the cast.
return new CSDateTime(csDate.AddMinutes(units), this.DateType);
Remove Unnecessary Using Directives
I always remove unnecessary using directives for a few reasons that I discuss in detail in my coding standards book and conference session. First, let me show the issue:
using log4net;
using System.Globalization;
using System.Diagnostics; // <- No longer in use
In this case the System.Diagnostics namespace is no longer used, so the solution is to just remove it. You can set it up making sure this is kept clean and sorted by using CodeRush, or the code cleanup fixes in Visual Studio. The biggest reason I keep this cleaned up is so I can remove dependencies! CodeRush and now Visual Studio can detect and prompt you to remove dependencies, but the usings need to be cleaned up first!
Add Braces in C#
Not adding braces {} to if statements can cause issues and readability issues. This code causes a violation:
if (somelist[i] != other.somelist[i])
return false;
This is how you fix it:
if (somelist[i] != other.somelist[i])
{
return false;
}
Make sure that you always add braces to if statements.
Inline Variable Declaration
Starting with .NET 7, when calling a method with an out parameter, it’s advisable to inline the variable declaration. Here is an example of the issue:
double number1;
if(double.TryParse(dollarAmount, out number1))
To fix, just move the declaration to the parameter as shown below:
if(double.TryParse(dollarAmount, out double number1))
Using inline variable declarations can improve the performance of the code.
Use Pattern Matching to Avoid 'As' Followed by A 'Null' Check
The "is" keyword was added to the .NET Framework 2.0 and is the preferred way to avoid a null check of the object.
Here is an example of the issue:
var other = obj as CustomArgumentInstance;
if ((object)other == null)
{
return false;
}
This is how you fix the code violation:
if (obj is not CustomArgumentInstance other)
{
return false;
}
The fix makes your code cleaner and can affect performance since pattern matching combines type checking and extraction.
Always Use Accessibility Modifiers
Accessibility modifiers are used to control the visibility of accessibility of types and members and are critical for proper Object-Oriented Programming. The code base that I used for this article had an excessive number of missing modifiers, the most that I have ever seen. I write about this in my coding standards book.
Here are the accessibility modifiers available in .NET:
- public: This modifier allows the element to be accessed from any other code in the same assembly or in other assemblies that reference it.
- private: The private modifier restricts access to the containing type only. It means that the element can only be accessed from within the same class or struct.
- protected: The protected modifier allows access within the containing type and any derived types. It means that the element can be accessed from the same class, struct, or any derived classes, but not from unrelated classes or instances.
- internal: The internal modifier allows access from any code in the same assembly. It means that the element can be accessed from any code within the same assembly but not from code in other assemblies.
- protected internal: This modifier combines the behavior of both the protected and internal modifiers. It allows access from the same assembly and derived types.
- private protected: This modifier is a combination of the private and protected modifiers. It allows access within the containing type and from derived types that are in the same assembly.
Here are my general rules:
- Most classes should be marked as
internal. Only make them public if they are intended to be reused by another assembly or if a particular framework requires it.
- All fields must be marked as
private. It is important to maintain OOP encapsulation by limiting direct access to fields.
- Most methods, properties, delegates, and events should be marked as
private or internal. Only make them public if they are designed to be called by another assembly.
Remove Unread Private Members
As code is removed or changed in a class, that might create unused private members such as variables, methods, properties etc. It’s important to remove them to improve code readability and improve maintenance.
Often, when I encounter this issue, it is common to find a field being initialized in a constructor but never utilized within the class. The code base used for this article had this issue throughout the solution. This oversight can have implications for performance, especially depending on the nature of the variable in question.
The Importance of the readonly Modifier
The readonly modifier is used to designate fields or variables that can only be assigned once, typically during object creation or in the constructor. It is commonly employed in constructors for variables received from the calling code, such as database connections, ensuring their immutability throughout the object's lifespan. It’s very important that these are coded correctly and, in the code I reviewed for this article, it had too many missing readonly modifiers.
Use Compound Assignments
Compound assignments have been available since .NET was released and is the preferred way to modify numbers and strings. Here is an example of the issue:
var peopleCount = 0;
var loopedCount = 0;
foreach (var peoplePage in people.Page(10))
{
loopedCount++;
peopleCount = peopleCount + peoplePage.Count();
}
Here is how to fix it:
foreach (var peoplePage in people.Page(10))
{
loopedCount++;
peopleCount += peoplePage.Count();
}
Here is a list of all the compound assignment operators.
Arithmetic Compound Assignment Operators
- +=: Add and assign.
- -=: Subtract and assign.
- *=: Multiply and assign.
- /=: Divide and assign.
- %=: Modulo and assign.
Bitwise Compound Assignment Operators
- &=: Bitwise AND and assign.
- |=: Bitwise OR and assign.
- ^=: Bitwise XOR and assign.
- <<=: Left shift and assign.
- >>=: Right shift and assign.
Use the Index Operator
C# 8 introduced the index-from-end operator when accessing items in a collection. Here is an example of this prior to C# 8:
return results[results.Count - 1];
In this case, this is how this would be improved by using the index operator:
return results[^1];
Remove Unnecessary Expression Values
When coding, it is crucial not to overlook or discard the value returned by a method. Neglecting the return value is not just a matter of style; it has implications that go beyond code readability and cleanliness. My concerns are performance, introducing confusion during the debugging process and making maintenance more challenging. This is an example of the issue:
_memoryCache.Set(key, obj, new TimeSpan(0, minutes, 30));
The _memoryCache is an instance of IMemoryCache. To fix, use a discard as shown below:
_ = _memoryCache.Set(key, obj, new TimeSpan(0, minutes, 30));
Discards, like unassigned variables, lack a value. However, using discards serves as a communication tool for both the compiler and other developers reading your code. It explicitly indicates your intention to disregard the result of an expression. Discards can be a beneficial practice.
Remove Unnecessary Value Assignments
For performance, it’s very important to remove unnecessary value assignments and need to be cleaned up. Here is an example of the issue:
List<People> users = null;
int count = 0;
The default value for reference types is null, and for integers, it is zero. Therefore, there is no need to explicitly set these values in your code. To fix this, simply remove the assignment as shown below:
List<People> users;
int count;
Only assign a value if it’s different from the default value. This cleanup can be performed by CodeRush during the saving of a file along with one of the code cleanup fixes in Visual Studio.
Remove Unused Parameters
Unused parameters in methods can lead to numerous issues and should be consistently removed. During my review of the code base for this article, I encountered a sizable number of such problems, which required a significant amount of time to rectify. To illustrate this matter, consider the following example:
public OrderController(ILogger<OrderController> logger, IRouter router)
{
_webRouter = router;
}
As you can see, the parameter "logger" is not used, so to fix it just remove it!
public OrderController(IRouter router)
{
_webRouter = router;
}
To address this issue, I utilize CodeRush, which not only eliminates the unused parameter but also refactors the code in all instances where the method is invoked. This makes the process straightforward and efficient.
Proper Using Directive Placement
In .NET, there are multiple places you can place using directives in a code file. The coding standard is to place them outside of the namespace. This is an example of namespace placement in Spargine OSS.
![Proper Using Directive Placement]()
The primary reason for me is easier refactoring. Firstly, Visual Studio automatically positions the using directive outside of the namespace. Secondly, both Visual Studio and CodeRush offer refactoring capabilities that can sort and remove unused namespaces. However, these features only function properly if the using directives are placed outside of the namespace.
Use the Switch Expressions Instead of Statements
Switch expressions were introduced in .NET Core 3 and is now the recommended approach for implementing switch statements due to several advantages. Consider the following example of a switch statement:
public enum DayOfWeek
{
Monday,
Tuesday,
Wednesday,
Thursday,
Friday,
Saturday,
Sunday
}
public string GetDayOfWeekName(DayOfWeek dayOfWeek)
{
string dayName;
switch (dayOfWeek)
{
case DayOfWeek.Monday:
dayName = "Monday";
break;
case DayOfWeek.Tuesday:
dayName = "Tuesday";
break;
case DayOfWeek.Wednesday:
dayName = "Wednesday";
break;
case DayOfWeek.Thursday:
dayName = "Thursday";
break;
case DayOfWeek.Friday:
dayName = "Friday";
break;
case DayOfWeek.Saturday:
dayName = "Saturday";
break;
case DayOfWeek.Sunday:
dayName = "Sunday";
break;
default:
dayName = "Invalid day";
break;
}
return dayName;
}
Although many developers have traditionally used the conventional switch statement syntax since the inception of .NET, a more modern and concise approach is available using switch expressions. By adopting switch expressions, we can rewrite the code as follows:
public string GetDayOfWeekName(DayOfWeek dayOfWeek)
{
string dayName = dayOfWeek switch
{
DayOfWeek.Monday => "Monday",
DayOfWeek.Tuesday => "Tuesday",
DayOfWeek.Wednesday => "Wednesday",
DayOfWeek.Thursday => "Thursday",
DayOfWeek.Friday => "Friday",
DayOfWeek.Saturday => "Saturday",
DayOfWeek.Sunday => "Sunday",
_ => "Invalid day"
};
return dayName;
}
This is much cleaner and more streamlined. One notable advantage of using switch expressions is that we no longer need to concern ourselves with adding break statements. In the past, it was common to overlook or accidentally omit breaks in switch statements, resulting in build errors. However, with switch expressions, this concern is eliminated.
Use Pattern Matching
Pattern matching was introduced in the C# programming language with the release of C# 7 and has since become the preferred approach for checking the shape or structure of data. Since its initial release, pattern matching has been widely used in coding. Here is an example of code prior to pattern matching being introduced in C#:
if (result.ObjectType == ProjectTypes.Boolean || result.ObjectType == ProjectTypes.String)
{
return (result.ToString() == "true");
}
Here is how you would refactor this using pattern matching:
if (result.ObjectType is ProjectTypes.Boolean or ProjectTypes.String)
{
return (result.ToString() == "true");
}
Pattern matching is much cleaner and easier to understand.
Summary
In this article, I have shown many common style issues that can also impact performance. The upcoming articles will delve deeper into more common issues, accompanied by detailed code examples and thorough explanations. Just about every issue discussed in this article can be quickly refactored by using the Visual Studio extension called CodeRush. CodeRush is free and can be downloaded by going here: https://www.devexpress.com/products/coderush/
For further guidance and insights, I highly recommend obtaining a copy of my book, "Rock Your Code: Coding Standards for Microsoft .NET" available on Amazon.com. Additionally, to explore more performance tips for .NET, I encourage you to acquire the 3rd edition of "Rock Your Code: Code & App Performance for Microsoft .NET" also available on Amazon.com.
To analyze your code using the same settings I used in these articles, I encourage you to incorporate my EditorConfig file. It can be found at the following link: https://bit.ly/dotNetDaveEditorConfig. I update this file quarterly, so remember to keep yours up to date as well. I hope you will check out my OSS project Spargine by using this link: https://bit.ly/Spargine.
Please feel free to leave a comment below. I would appreciate hearing your thoughts and feedback.