C# Bad Practices: Learn How to Make a Good Code by Using Examples of Bad Code – Part 3

Previous Articles from this Series

This is the third article of my series: C# Bad Practices: Learn how to make a good code by using examples of bad code.

You can find the previous two articles by clicking on the links below:

• C# Bad Practices: Learn How to Make Good Code by Using Examples of Bad Code
• C# Bad Practices: Learn How to Make a Good Code by Using Examples of Bad Code – Part 2

To understand this article, you don't have to read any of my previous articles, but I encourage you to do that. :)

The Goal of the Article

I would like to show my opinion on a very interesting topic:

When You Shouldn't Use Static Classes?

In the article, I'm considering using static classes as a part of a business logic implementation.

I came across many different opinions on this topic so I decided to present my point of view on that.

The same as in previous articles in this series, I will present the code which is creating some problems (in my opinion) - because of using static classes and then I will present my solution for solving these problems.

What should the reader know before starting to read the article:

• Basic knowledge of C# language
• Basic knowledge of Dependency Injection design pattern
• Basic knowledge of how IOC containers work
• Basic knowledge of unit testing
• Basic knowledge how mocking frameworks work

What is Static Class in C#?

Microsoft says:

A static class is basically the same as a non-static class, but there is one difference: a static class cannot be instantiated. In other words, you cannot use the new keyword to create a variable of the class type. Because there is no instance variable, you access the members of a static class by using the class name itself. For example, if you have a static class that is named UtilityClass that has a public method named MethodA, you call the method as shown in the following example:

C#

UtilityClass.MethodA();

And that's why many developers are using them very often. You don't need to create an instance of the class to use it, you just put a dot and voila, you can access its members. It's fast and clear and you save a time of object creating.

There are many rules for static classes, but I will not elaborate about them.

You can learn about them from the MSDN site:

MSDN

For this article, two rules of the static class are important:

• static class is sealed – you cannot inherit from it
• static class cannot implement an interface

Let's Go to the Code

The example below was created only to present the problem on a concrete code. Sending e-mails is not related to the problem described by this article, it's only showing a usage of static classes in a wrong way (IN MY OPINION :)).

Here is the code which I want to set as an example:

public class EmailManager
{
  public bool SendEmail(int emailId)
  {
    if (emailId <= 0)
    {
      Logger.Error("Incorrect emailId: " + emailId);
      return false;
    }

    EmailData emailData = DatabaseManager.GetEmailData(emailId);
    if (emailData == null)     
    {       
        Logger.Error("Email data is null (emailId: " + emailId + ")");       
        return false;     
    }

    EmailMessage emailMessage = EmailMessageCreator.CreateEmailMessage(emailData);

    if (!ValidateEmailMessage(emailMessage))
    {
      Logger.Error("Email message is not valid (emailId: " + emailId + ")");
      return false;
    }
 
    try
    {
      EmailSender.SendEmail(emailMessage);
      Logger.Info("Email was sent!");
    }
    catch(/* catch only exceptions, which you know that may occur and you can't 
    do anything to avid them, like: network related error, addressee server refused... etc.*/)
    {
      Logger.Exception(ex.ToString());
      return false;
    }

    return true;
  }
 
  private bool ValidateEmailMessage(EmailMessage emailMessage)
  {
    if(emailMessage == null) return false;

    if (string.IsNullOrEmpty(emailMessage.From) || string.IsNullOrEmpty(emailMessage.To) || 
    string.IsNullOrEmpty(emailMessage.Subject) || string.IsNullOrEmpty(emailMessage.Body)) 
    return false;
    
    if (emailMessage.Subject.Length > 255) return false;
    
    return true;
  }
}

Let's imagine that we have a component, which is sending e-mail messages. It is taking a message containing a message identificator from a queue (doesn't matter which implementation, might be MSMQ, RabbitMQ, etc.), creating an e-mail message and sending it to Addressee.

We have here manager class which takes as a parameter identificator of stored in the database message (messageId). It has its own logic like:

• guard clause
• validation of the message returned by EmailMessageCreator – it is divided to a private method – I'm treating it as a part of the SendEmail method of EmailManager class
• managing program flow
• error handling, as while sending e-mails, you can come across many known errors like:
  • network related error, addressee server refused... etc.

and is also calling a logic from another components like:

• DatabaseManager static class – a component responsible for getting message information from DB by messageId
• EmailMessageCreator static class – a component responsible for creating email message to send
• EmailSender static class – a component responsible for sending a message to Addressee via SMTP
• Logger static class – a component responsible for logging an information in a log file

To simplify a situation, assume that all the above classes are working like a services and there are no racing conditions here for their members.

It looks simple and clear. It's working, it's fast and we can easily, quickly implement it.

So what's wrong with this code??!!

I see a few problems in it..

Issues

Unit Testing

Imagine now that you want to write unit tests for EmailManager class, so you want to test own logic of SendEmail method: like:

• check what value is returned if:
  • input is incorrect
  • CreateEmailMessage will return invalid data
• check if exception is thrown by tested method if:
  • CreateEmailMessage method throws an exception
  • GetEmailData method throws an exception
  • SendEmail method from EmailSender class throws an exception
  • check if correct method of the Logger was called with proper parameters, in case:
    • input is incorrect
    • SendEmail method from EmailSender class has thrown exception
    • email was sent properly

This behavior should not change even if related classes will change their behavior.

You cannot unit test this class as there are hard dependencies to another classes like:

• DatabaseManager
• EmailMessageCreator
• EmailSender
• Logger

What's the problem with that?

While testing SendEmail, you will be testing their code as well. It will be integration test. In simple words, a bug in for example CreateEmailMessage should not affect a test result of the SendEmail method.

Furthermore, while testing own code of SendEmail method from EmailSender class, you don't want to call the database and send real e-mail as you're not testing them and unit test has to be fast!

Another thing is that you're not able to check in your test, if the Logger class was called or not, because you're not able to mock the Logger class.

IMPORTANT

After Wonde Tadesse comment, I can agree that you can use Microsoft Fakes framework, to unit test SendEmail method from EmailSender class(version with dependencies to static classes). But:

• (in the original version with dependencies to static classes) you still violate Dependency Inversion principle from SOLID principles, so this is a bad practice - EmailManager decide which implementation of related class to take.
• This feature is only available with the highest versions of Visual Studio (Premium, Ultimate or Enterprise). Your company is dependent on a concrete version of Visual Studio now. Is it a good practice?

  Even if your company has the highest version of VS bought for every developer (very rare, and unlikely), what if the company will decide then to downgrade version of Visual Studio to Professional? Your tests will fail and you will have to refactor your code anyway. Great practice?

Future Changes

Imagine now that after some period of time, EmailSender class needs a different behavior of the EmailMessageCreator but just a little bit. So you just want to change 10% of current EmailMessageCreator code and the rest can stay as is.

But at the same time, you don't want to break unit tests written for this class and you want to follow Open/Closed Principle from SOLID principles.

IMPORTANT

Wikipedia says about Open/Closed Principle:

software entities (classes, modules, functions, etc.) should be open for extension, but closed for modification

We want to keep both EmailMessageCreator and EmailMessageCreatorExtended classes, also because there is another consumer of EmailMessageCreator which want to keep using its old version without modification.

We also don't want to touch the caller, in our example - EmailManager class.

Unfortunately, we can't achieve that, because:

• if we want to keep the current implementation of EmailMessageCreator and only extend it, the best choice would be to create a class EmailMessageCreatorExtended (forgive me for this name, it's only to differentiate them) which will inherit from the EmailMessageCreator class. We can't do it, as static class are sealed so you can't inherit from them.
• You also cannot replace the implementation of EmailMessageCreator class without changing the caller, as static classes cannot implement an interface. So even if you are injecting EmailMessageCreator via constructor of EmailManager class, to change it to use another static class like EmailMessageCreatorVersion2 (forgive me for this name, it's only to differentiate them) you will have to change the code of the caller.

How to Fix That??!!

Well, the solution is VERY SIMPLE.

First Step

Just remove static from the following classes:

• DatabaseManager
• EmailMessageCreator
• EmailSender
• Logger

and from their methods!

Second Step

Create an interface for each of the static classes:

• IDatabaseManager
• IEmailMessageCreator
• IEmailSender
• ILogger

and make them implement those interfaces.

Third Step

Change implementation of the EmailManager class as below:

public class EmailManager
{
  private readonly IDatabaseManager _databaseManager;
  private readonly IEmailMessageCreator _emailMessageCreator;
  private readonly IEmailSender _emailSender;
  private readonly ILogger _logger;
  public EmailManager(IDatabaseManager databaseManager, 
  IEmailMessageCreator emailMessageCreator, IEmailSender emailSender, ILogger logger)
  {
    _databaseManager = databaseManager;
    _emailMessageCreator = emailMessageCreator;
    _emailSender = emailSender;
    _logger = logger;
  }
 
  public bool SendEmail(int emailId)
  {
    if (emailId <= 0)
    {
      _logger.Error("Incorrect emailId: " + emailId);
      return false;
    }

    EmailData emailData = _databaseManager.GetEmailData(emailId);
    if (emailData == null)     
    {       
      _logger.Error("Email data is null (emailId: " + emailId + ")");       
      return false;     
    }

    EmailMessage emailMessage = _emailMessageCreator.CreateEmailMessage(emailData);
 
    if (!ValidateEmailMessage(emailMessage))
    {
      _logger.Error("Email message is not valid (emailId: " + emailId + ")");
      return false;
    }
 
    try
    {
      _emailSender.SendEmail(emailMessage);
      _logger.Info("Email was sent!");
    }
    catch (/* catch only exceptions, which you know that may occur and you can't 
    do anything to avid them, like: network related error, addressee server refused... etc.*/)
    {
      _logger.Exception(ex.ToString());
      return false;
    }

    return true;
  }
 
  private bool ValidateEmailMessage(EmailMessage emailMessage)
  {
    if(emailMessage == null) return false;

    if (string.IsNullOrEmpty(emailMessage.From) || string.IsNullOrEmpty(emailMessage.To) || 
    string.IsNullOrEmpty(emailMessage.Subject) || string.IsNullOrEmpty(emailMessage.Body)) 
    return false;
    
    if (emailMessage.Subject.Length > 255) return false;

    return true;
  }
}

Fourth Step

Now you can set up one of the IOC containers like Ninject or Autofac for your project, and just bind the above implementations to adequate interface, like:

DatabaseManager to IDatabaseManager

in your IOC container configuration.

Here is, for example, a wiki page which describe how to create a configuration for Ninject:

• Dependency Injection With Ninject

IOC container will inject then proper implementations into the EmailManager class.

You can inject implementations to EmailManager class manually as well.

And voila, we've got it!

What Did We Gain After This Change?

Unit Testing Problem - SOLVED

So, do you remember our first problem with unit testing? Now we can unit test our EmailManager class without any problems. We can inject into it any implementation we want, mock, stub, etc.

We can use mocking framework like Moq, to mock classes:

• DatabaseManager – then, for the GetEmailData method we can:
  • bypass connecting to the database,
  • return whatever value we want from memory,
  • simulate throwing exception – to check how our SendEmail method from EmailManager class will behave,
• EmailMessageCreator – then, for the CreateEmailMessage method we can:
  • return whatever value we want – to check how our SendEmail method from EmailManager class will behave
  • simulate throwing exception – to check how our SendEmail method from EmailManager class will behave
• EmailSender – then, for its SendEmail method we can:
  • bypass sending emails
  • return whatever value we want – to check how our SendEmail method from EmailManager class will behave
  • simulate throwing exception – to check how our SendEmail method from EmailManager class will behave
• Logger – then, for its Info, Error and Exception methods, we can:
  • check if they were called and if yes, with what parameters – it will allow us to check if the information about event or that exception occurred is logged or not.

I will not present an API of Moq library as it isn't the main topic of this article, and it may be a topic for the separate article. It would distract from the main topic of the article which is usage of the static classes. All you need to implement above mocks in your unit tests is mentioned here:

• Moq Quickstart

Future Changes Problem - SOLVED

The next problem we had was a lack of ability to extend static classes and a lack of ability to change implementation of related classes without changing the consumer code.

Do we still have these issues in our new code?

NO.

Why?

Because, thanks to programming to abstraction (introducing interfaces), dependency injection and removing static from related classes, we can now:

• extend functionality of related classes using inheritance
• replace implementation of related classes in the caller (SendEmail method of EmailManager class) in IOC container configuration without touching the caller at all

Static Classes Will Be faster...

How could I not say about the difference between those two solutions in case of performance?

People who are implementing solutions similar to example with static classes given in the article will say:

“Static classes will be faster as while using them, you don't have to create an instance of the class on every call to its method”.

But...

In the solution proposed by myself, do we really need to create an instance of the class every time we want to use a method from it?

Obviously NO.

We can just create an object at the application start and treat it as a singleton. It can be very easily achieved by configuration of IOC container. In all of its implementations like Ninject, Autofac, etc. you can set an object lifecycle to a singleton and that's it. Everytime, we will want to access the implementation of the interface, IOC container will return the same object.

Here, you can find an information about object lifecycles in Ninject:

• Ninject Object Lifecycle

It's extremely easy as you will have to only add one method call to each Interface binding:

.InSingletonScope()

But if we don't want to use any ready to use implementations of IOC container, we can implement singleton by ourselves.

To be 100% honest, there is one other factor, which is the fact that we replaced also static method to instance methods. Does it change anything? Let's check what Microsoft says about it:

Microsoft says:

A call to a static method generates a call instruction in Microsoft intermediate language (MSIL), whereas a call to an instance method generates a callvirt instruction, which also checks for a null object references. However, most of the time the performance difference between the two is not significant.

In my opinion, unless there we have a huge performance problem, we shouldn't care about it. We are gaining much for a very little (very likely invisible) performance decrease.

Where Should We Use Static Classes Then?

So should we resign from static classes at all? Of course NO.

In my opinion, we can implement a business using static class, if:

• we don't care about unit tests – don't say it loudly :) and we are sure that we will not modify the code – it is the final version and it won't be extended

OR

• it is extremely simple project – only few classes and we want to keep it as simple as possible.

OR

• you're implementing an extremely simple tool which has no reason to be modified or extended and has no side-effects, like modification of state or exception throwing - in fact, it's extremely hard to find this kind of situation in a real world. For example, almost always you will have to validate an input and often throw an exception if it is invalid.
  IMPORTANT: In case of built in framework static classes containing some basic tools, like System.Math, etc.:

  I'm treating them as an exception to the rule. I'm thinking about them as built in language instructions. They are tested properly by Microsoft, they won't be modified, so it's completely acceptable to use them in my opinion.

We can also use static classes to implement constants for reference projects. For example:

public class Point
{
  public Point (int x, int y)
  {
    X = x;
    Y = y;
  }
  public int X{get;private set;}
  public int Y{get;private set;}
}
 
public static class Constants
{
  public static Point StartPoint = new Point(0, 0);
}

Conclusion

I'm always trying to create a code as flexible as possible. If you will implement suggested by myself, “new” solution instead of using static classes, you won't lose anything (if we skip a very little, probably invisible performance decrease) and at the same time you can gain much, like unit testable code and flexibility for the future changes. This ability will be extremely beneficial for the huge, long-term projects.

So to sum up, I would say:

“Avoid using static classes while implementing a business logic of the application unless there is a strong reason for using them!”.

Thanks!

If you have any questions related to the article, don't hesitate to contact me!

Sources

Used in the article information:

• https://en.wikipedia.org
• https://msdn.microsoft.com

Used in the article images:

• https://openclipart.org

History

• 12^th December, 2016: Initial version