Click here to Skip to main content
13,803,077 members
Click here to Skip to main content
Add your own
alternative version

Tagged as

Stats

83.9K views
119 bookmarked
Posted 12 Dec 2016
Licenced CPOL

C# BAD PRACTICES: Learn how to make a good code by bad example – Part 3

, 4 Jan 2017
Rate this:
Please Sign up or sign in to vote.
WHEN YOU SHOULDN'T USE STATIC CLASSES?

Previous articles from this series

Hi! This is the third article of my series: C# BAD PRACTICES: Learn how to make a good code by bad example.
Previous two you can find here:

C# BAD PRACTICES: Learn how to make a good code by bad example

C# BAD PRACTICES: Learn how to make a good code by bad example – Part 2

To understand this article, you don't have to read any of my previous articles, but I encourage 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 making some problems (in my opinion) - because of using static classes and then I will present my solution for solving these problems.

 

What should reader know before start reading 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:

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, 2 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 devided 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 above classes are working like a services and there is 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.

IMPORTANT: Remember that unit test IS NOT an integration test. It should test only isolated piece of code. If there are relations to other pieces of code, then they should be replaced by mocks, stubs etc.

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 a simple words, a bug in for example CreateEmailMessage should not affect a test result of the SendEmail method.

IMPORTANT: I don't want to say that we only need unit tests. Integration tests are also important and we should have them both, but we should separate them clearly. Why? Because all Unit Tests should always pass! Integration tests usualy need some aditional configuration, like fake database, fake smtp server, etc.

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 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 have 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 current implementation of EmailMessageCreator and only extend it, the best choice would be to create a class EmailMessageCreatorExtended (forgive me 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 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, 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 manualy 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

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 this issues in our new code?

NO.

Why?

Because, thank 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 proposed by myself solution, 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 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 a built in a 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:

Used in the article images:

 

License

This article, along with any associated source code and files, is licensed under The Code Project Open License (CPOL)

Share

About the Author

Passionate, Microsoft Certified Software Developer(Senior),
having Masters Degree in Information Technology.

You may also be interested in...

Pro

Comments and Discussions

 
QuestionStatics are evil because race conditions Pin
thund3rstruck6-Mar-17 4:32
memberthund3rstruck6-Mar-17 4:32 
AnswerRe: Statics are evil because race conditions Pin
Radosław Sadowski7-Mar-17 2:49
memberRadosław Sadowski7-Mar-17 2:49 
GeneralRe: Statics are evil because race conditions Pin
thund3rstruck7-Mar-17 3:28
memberthund3rstruck7-Mar-17 3:28 
GeneralRe: Statics are evil because race conditions Pin
Radosław Sadowski7-Mar-17 3:35
memberRadosław Sadowski7-Mar-17 3:35 
GeneralWhy is there not a static implementation ... Pin
MComeauDigx11-Jan-17 4:10
memberMComeauDigx11-Jan-17 4:10 
GeneralRe: Why is there not a static implementation ... Pin
Radosław Sadowski11-Jan-17 4:41
memberRadosław Sadowski11-Jan-17 4:41 
GeneralRe: Why is there not a static implementation ... Pin
MComeauDigx11-Jan-17 7:40
memberMComeauDigx11-Jan-17 7:40 
GeneralRe: Why is there not a static implementation ... Pin
Radosław Sadowski11-Jan-17 7:51
memberRadosław Sadowski11-Jan-17 7:51 
QuestionMy vote for 5 Pin
Kaimar7-Jan-17 10:40
memberKaimar7-Jan-17 10:40 
AnswerRe: My vote for 5 Pin
Radosław Sadowski7-Jan-17 11:56
memberRadosław Sadowski7-Jan-17 11:56 
Suggestionit's all about side effects Pin
sx200830-Dec-16 8:36
membersx200830-Dec-16 8:36 
GeneralRe: it's all about side effects Pin
Radosław Sadowski31-Dec-16 5:57
memberRadosław Sadowski31-Dec-16 5:57 
GeneralRe: it's all about side effects Pin
sx20083-Jan-17 9:01
membersx20083-Jan-17 9:01 
GeneralRe: it's all about side effects Pin
Radosław Sadowski3-Jan-17 9:23
memberRadosław Sadowski3-Jan-17 9:23 
Question[My vote of 1] Too bad that CodeProject people erase all comments with critics Pin
ИздиславИздиславов29-Dec-16 7:54
memberИздиславИздиславов29-Dec-16 7:54 
GeneralMy vote of 5 Pin
LinaLamotte27-Dec-16 22:18
memberLinaLamotte27-Dec-16 22:18 
GeneralRe: My vote of 5 Pin
Radosław Sadowski29-Dec-16 2:58
memberRadosław Sadowski29-Dec-16 2:58 
PraiseNice article Pin
Member 1065102625-Dec-16 22:31
memberMember 1065102625-Dec-16 22:31 
GeneralRe: Nice article Pin
Radosław Sadowski26-Dec-16 12:13
memberRadosław Sadowski26-Dec-16 12:13 
PraiseGreat Pin
MM199325-Dec-16 15:11
memberMM199325-Dec-16 15:11 
GeneralRe: Great Pin
Radosław Sadowski26-Dec-16 12:11
memberRadosław Sadowski26-Dec-16 12:11 
QuestionThe third step has a typo. Pin
Member 1043760124-Dec-16 3:50
memberMember 1043760124-Dec-16 3:50 
AnswerRe: The third step has a typo. Pin
Radosław Sadowski25-Dec-16 4:10
memberRadosław Sadowski25-Dec-16 4:10 
QuestionIt seems to me that you are confusing inheritance with implementation Pin
Jorge Varas20-Dec-16 11:02
memberJorge Varas20-Dec-16 11:02 
AnswerRe: It seems to me that you are confusing inheritance with implementation Pin
Radosław Sadowski20-Dec-16 11:46
memberRadosław Sadowski20-Dec-16 11:46 

General General    News News    Suggestion Suggestion    Question Question    Bug Bug    Answer Answer    Joke Joke    Praise Praise    Rant Rant    Admin Admin   

Use Ctrl+Left/Right to switch messages, Ctrl+Up/Down to switch threads, Ctrl+Shift+Left/Right to switch pages.

Permalink | Advertise | Privacy | Cookies | Terms of Use | Mobile
Web06 | 2.8.181215.1 | Last Updated 4 Jan 2017
Article Copyright 2016 by Radosław Sadowski
Everything else Copyright © CodeProject, 1999-2018
Layout: fixed | fluid