Common Code Smells Mistake In C# - Part Two

Code smell is a word given to indicate a deeper problem in our programming code. Here, we will discuss about some of the code smell vulnerabilities that developers commonly face but don't recognize sometimes

Introduction

 
This is the second part of my article. Refer to my first article Common code smells mistake in C#, Part one. of this series for better understanding of the code smell bugs and vulnerabilities and some code smell bugs and their solutions.
 
Today we will go through some more programming code smells and we will also see how to avoid such bugs or vulnerabilities in our code.
 

Field assigned only in constructor must be readonly

 
If a class has field whose values are assigned only in the constructor throughout the program then that field must be readonly. readonly fields can only be assigned in constructor.
 
Noncompliant code
  1. public class Employee  
  2. {  
  3.     private string  _name;  // Noncompliant  
  4.   
  5.     Employee(string name)  
  6.     {  
  7.         _name= name;  
  8.     }  
  9. }  
Compliant Code
  1. public class Employee  
  2. {  
  3.     private readonly string  _name;   
  4.     Employee(string name)  
  5.     {  
  6.         _name= name;  
  7.     }  
  8. }  

Field marked with 'static readonly' should be const instead

 
The value for const field is computed at compile time while for static readonly it is computed at run time. As value computed at run time for const improves the performance so use const instead of static readonly fields.
 
Noncompliant code
  1. public class Demo  
  2. {  
  3.     static readonly int x = 5;  // Noncompliant  
  4.     static readonly bool isTrue = true// Noncompliant  
  5.     static readonly string str = "Bar";  // Noncompliant  
  6. }  
Compliant Code
  1. public class Demo  
  2. {  
  3.     const  int x = 5;   
  4.     const bool isTrue = true;  
  5.     const string str = "Bar";   
  6. }  

Avoid usage of protected member in sealed class

 
Private member is accessible only in the scope of same class, while protected members are accessible in the same class and child class.
 
Protected member in child class becomes private. As we know, sealed class cannot be inherited and it cannot have child class so creating protected member in that class is useless and confusing.
 
Noncompliant code
  1. public sealed class SealedClassDemo  
  2. {  
  3.     protected string _name = "Test";  // Noncompliant  
  4.     protected void SetName(string name) // Noncompliant  
  5.     {  
  6.         // ...  
  7.     }  
  8. }  
Compliant code
  1. public sealed class SealedClassDemo  
  2. {  
  3.     private string _name = "Test";  
  4.     public void SetName(string name)  
  5.     {  
  6.         // ...  
  7.     }  
  8. }  
Avoid usage of goto statement
  1. public string GetTitle(string gender,bool isMarried)  
  2. {  
  3.   if (gender.Equals("Male"))  
  4.   {  
  5.     return "Mr. ";  
  6.   }  
  7.   return p.IsMarried ? "Mrs. " : "Miss ";  
  8.  
Goto is an unstructured type of control flow statement. It makes the code complex and harder to maintain. We can use structured control flow statement such as if for while etc. instead of goto to make our code easy to read and maintain.
 

Ternary operators should not be nested

 
Ternary operators are easy to write but when it comes to maintenance or debugging ternary operators become complex to understand. Instead of nested ternary operator write it as separate statement.
 
Noncompliant code
  1. public string GetTitle(string gender,bool isMarried)  
  2. {  
  3.   return gender.Equals("Male") ? "Mr." : isMarried ? "Mrs. " : "Miss ";  // Noncompliant  
  4. }  
Compliant code
  1. public string GetTitle(string gender,bool isMarried)    
  2. {    
  3.     if(gender.Equals("Male"))  
  4.     {  
  5.         return "Mr.";  
  6.     }  
  7.     return isMarried ? "Mrs. " : "Miss ";  
  8. }   

Use culture for string operations

 
String operations without culture may work fine in a local environment. But bugs may come for international character or different encoding.
 
Specifying culture in string operations removes the ambiguity of code so that it can be used for any encoding or language.
 
Following string method are culture dependent,
  • ToLower();
  • ToUpper();
  • IndexOf();
  • LastIndexOf();
  • Compare();
All methods mentioned above have overloaded method which accept an argument to specify the culture.
 
Noncompliant code
  1. var strLower= str.ToLower(); //Noncompliant  
Compliant code
  1. var strLower= str.ToLower(CultureInfo.InvariantCulture);  
  2.   
  3. // or  
  4.   
  5. var strLower= str.ToLowerInvariant();  
Note
string.CompareTo() is also culture dependent but it doesn't have any overloaded method to specify the culture so we can CompareOrdinal() or Common() method instead.
 
Thank you for reading this article, I will post more such code smells in my next article.