Common Code Smells Mistake In C# - Part Two

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

public class Employee
{
    private string  _name;  // Noncompliant

    Employee(string name)
    {
        _name= name;
    }
}

Compliant Code

public class Employee
{
    private readonly string  _name;
    Employee(string name)
    {
        _name= name;
    }
}

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

public class Demo
{
    static readonly int x = 5;  // Noncompliant
    static readonly bool isTrue = true; // Noncompliant
    static readonly string str = "Bar";  // Noncompliant
}

Compliant Code

public class Demo
{
    const  int x = 5;
    const bool isTrue = true;
    const string str = "Bar";
}

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

public sealed class SealedClassDemo
{
    protected string _name = "Test";  // Noncompliant
    protected void SetName(string name) // Noncompliant
    {
        // ...
    }
}

Compliant code

public sealed class SealedClassDemo
{
    private string _name = "Test";
    public void SetName(string name)
    {
        // ...
    }
}

Avoid usage of goto statement

public string GetTitle(string gender,bool isMarried)
{
  if (gender.Equals("Male"))
  {
    return "Mr. ";
  }
  return p.IsMarried ? "Mrs. " : "Miss ";
}

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

public string GetTitle(string gender,bool isMarried)
{
  return gender.Equals("Male") ? "Mr." : isMarried ? "Mrs. " : "Miss ";  // Noncompliant
}

Compliant code

public string GetTitle(string gender,bool isMarried)
{
    if(gender.Equals("Male"))
    {
        return "Mr.";
    }
    return isMarried ? "Mrs. " : "Miss ";
}

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

var strLower= str.ToLower(); //Noncompliant

Compliant code

var strLower= str.ToLower(CultureInfo.InvariantCulture);

// or

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.

 

<<Previous Part: Common code smells mistake in C#