What is wrong with Static Classes?

There are many concepts in computer programming which are commonly recommended against but rarely does anyone explain you why things like goto, singleton, static classes, etc can cause problems if not used with care. Today we’re going to see:

• What kind of problem can a static class introduce
• When it makes sense to create a static class
• How to refactor the code which was using a static class

So to cover all of the above points let us take the example of our imaginary programmer friend ‘Tim’.

Tim was asked to write a product order placement component. One of the features of that component was that it could send email confirmation of the order to the user so he wrote something like the following:

    class OrderSystem
    {
        public void AddOrder(User user, Product product)
        {
            // Persist the order in DB

            Email.Send(user.Email, "Order confirmation", "Your order has been placed");
        }
    }

As you can see he created a static class Email with a Send method that takes bunch of arguments and sends the email. Lets see what was he thinking when he wrote this.

• This is the simplest way to do it. Very clean one line code to send emails.
• I don’t need ‘instances’ of the mail class. I just want to send email. Why create instance?
• If there isn’t going to be any instance then lets make it a static class.

Later his supervisor asked him to write a unit test for this.

• When he began writing a unit test he couldn’t verify that email was sent in a easy and straightforward way.

          [TestMethod]
          void AddOrderTest()
          {
              var orderSystem = new OrderSystem();
              var user = new User(1, "john", "john@example.com");
              var product = new Product(101, "pepsi");
              orderSystem.AddOrder(user, product);
              var order = orderSystem.GetOrder(user, product);
              Assert.NotNull(order);
              Assert.... // What do I write here to check if email is sent? Should I log into inbox and pull his emails?
          }
  

• He went ahead and actually wrote code to login to mailbox of user and see if the mail has arrived but then he realized he is not only testing his OrderSystem but also the mail delivery system. If the mail delivery system fails then his unit test fails. Failing test would mean there is something wrong with his order system but in reality the problem could either be in the network, the smtp server or somewhere else in the universe between the mailbox and his system.
• Then he passed on the code to his colleague for review. The colleague first of all wanted to evaluate the design so he collapsed the code blocks and just looked at the interface. It wasn’t immediately clear that this component also has a dependency on email subsystem. Why? Because the implementation secretly and directly talks to the Email static class. Email subsystem doesn’t have to be injected into it anywhere. It doesn’t go in the constructor or in any property. It doesn’t seem that this system sends an email at all. Its after you look at the implementation of the class, when you realize that this class also has a dependency on another class.

Some people might say. Oh well! We don’t write unit tests and we don’t mind looking at implementation of classes to understand whats going on. We’re after all not the most productive team on earth. We take our time in fixing bugs and understanding code after all thats what our infinite sprint cycles are for. We’re ‘Agile’.

Nevermind, the story continues. Later Tim is asked to make it so that we either send sms or email confirmation based on the settings of the user.
If he changes the code to add an ‘IF’ statement then he is introducing two code paths in his method.

        public void AddOrder(User user, Product product)
        {
            // Persist the order in DB
            if (user.ConfirmationMethod == ConfirmationMethod.Email)
                Email.Send(user.Email, "Order confirmation", "Your order has been placed");
            else
                Sms.Send(user.Mobile, "Your order has been placed");
        }

• The more number of distinct code paths a method has the more tests you have to write to cover all the code paths and ensure correctness.
• The more number of distinct code paths a method has the more chances of bugs you have due to variation of code paths and states.
• Now his test also fails if there is problem in SMS sending subsystem hinting as if there is a problem in his method.

In reality Tim was really asked to introduce a new mechanism of confirmation message sending but he wen’t and modified the order system class which is irrelevant but he ‘had to’ because the mechanism of sending email was ‘baked’ into the implementation.

Some may argue at this point, “Well he could have faced all these problems even if the class was not static”. Yes that’s right but could he arrive at the right solution keeping the class or his mentality static?

So Tim realized that sending a confirmation message is something that needs to be abstracted out. He needs to be able to plug in the right implementation and he should be able to solve most of the issues related to adding new ways of delivery, reduced cyclomatic complexity, unit testing etc. He was right in his thoughts up to this point but he was somehow obsessed with static classes and loved the simplicity of being able to do something in one line without creating any objects or anything so he chose a wrong solution to the problem which was something like the following:

    abstract class OrderSystem
    {
        public void AddOrder(User user, Product product)
        {
            // Persist the order in DB
            SendConfirmation(user);
        }

        protected abstract void SendConfirmation(User user);
    }

As you can see he turned his class into an abstract class with a SendConfirmation method. Now he thought he would create one derived class for sending email and one for sending sms and he can mock this easily for unit testing purpose also.

Some may say, “Fair enough! Guy has a decent solution to the problem.”

So when you go to a restaurant do you have different kinds of waiters for taking orders for different kind of food?
Umm I want to eat vegetable so I’ll call that waiter wearing green shirt. Oh you want some meat? Lets call that waiter wearing red shirt. Doesn’t make sense right? The job of the waiter is to note down the dish you want to eat. The change of choice of dish for each customer is abstracted out in something called ‘order menu’. You don’t create different kind of waiters for different kind of dishes.

So he solved the problem by further damaging the design. Besides what would you do if you want to send sms and email both?

Alright so whats the ‘right’ way of solving the problem? The answer should be clear by now. We need to abstract out confirmation mechanism into a non static class like so:

    abstract class ConfirmationSender
    {
        public abstract void SendConfirmation(User user);
    }

    class SMSConfirmationSender: ConfirmationSender
    {
        public override void SendConfirmation(User user)
        {
            // Send sms here how ever you want
        }
    }

    class EmailConfirmationSender : ConfirmationSender
    {
        ....
    }

• Now his AddOrder method can take the ConfirmationSender as a parameter or in the constructor of the class depending upon whether the mechanism changes per user or based on system setting.
• From the interface of the class it is very clear that this class has a dependency on another class. You don’t need to look at the implementation of the method to realize it also has to send confirmation via email or sms.
• You can unit test this method without having to actually check mailbox of the user.
• Your test will no longer fail because of problem in email delivery mechanism because unit test will mock the ConfirmationSender and only verify if the confirmation was sent or not.
• For adding new confirmation delivery mechanisms you don’t have to modify the OrderSystem. The less this code gets modified the more stable and bug free it will be.
• The cyclomatic complexity will not increase with each new confirmation mechanism. Or even if you don’t want to send confirmation at all, you can just create a NullConfirmationSender that doesn’t do anything and just feed that to the method.

So like I said earlier. He could have all these problems even if the email sender was not a static class but if it is a static class it encourages all the wrong patterns and introduces the problems.

Some may say, “Ok so static class is evil then?” No! It does make sense to make a class static when

• It doesn’t have any state to maintain.
• The methods are very simple unit of operations that are ‘universal truth’ and won’t ever change.

The best example of perfect static class and method is Math class with methods like Min(num1, num2), Max(num1, num2), Sqrt(num), Abs(num), Sin(num).
Definition of Minimum, Maximum, Sin, Cos, etc. will never change. They are very small, highly concise fast operations. Whether a class has dependency on Math class at all, doesn’t even matter.

Some may say, “I’ve seen static classes with state in code written by top notch programmers”. Well every rule has an exception. You need to be able to judge what makes sense in what scenario, how much you care, how much time you have, how much ‘agile’ your team is, etc.

You might ask, “What about singletons? Shouldn’t we make a class static when there is only one instance in our universe for that?”. No! First of all the fact you need to have a single instance in your modeled universe, has nothing to do with whether class should be static or not. Static means “I don’t have state. I’m a helper.” If you really really want a single instance throughout your application you can opt for Singleton pattern.

“Why two times ‘really’ in your statement?”, you might ask. Well because such a strict definition of single instance can limit the possibility to have parallel worlds in your application and also you can still enforce single instance by using dependency injection framework and correct life time hints while registeration instead of making the class singleton. If you don’t use dependency injection and you want to make it clear by ‘design’ that you don’t want more than one instance lurking around in the app then you can opt for the Singleton.

---

Share your thoughts