Should I use static classes for methods that will do common tasks and would be called through out my application?

Question

I have spent the last few hours reading up on the use of static classes and trying to figure out if I should be using them or not but still have not come to any sort of conclusion. It seems that the argument could go either way. In my application I have created what I call "helper classes" which contains methods that will do very common tasks for me and would be called through out my application (ASP.Net MVC Web App using C#) and the simple question is, should they really be static or not?

Here is an example of one of my helpers.

public static class ActiveDirectoryHelper
{
    public static PrincipalContext GetPrincipalContext(string ouName)
    {
        var fullOUName = string.Concat("OU=", ouName,",DC=");

        return new PrincipalContext(ContextType.Domain, "", fullOUName, ConfigurationManager.AppSettings["ServiceAccountUser"], ConfigurationManager.AppSettings["ServiceAccountPassword"]);
    }

    public static PrincipalSearcher GetAllUsersInOU(string ouName)
    {
        var principalContext = GetPrincipalContext(ouName);
        var userPrincipal = new UserPrincipal(principalContext);
        return new PrincipalSearcher(userPrincipal);
    }

    public static UserPrincipal GetUserPrincipal(string userName, string ouName)
    {
        var principalContext = GetPrincipalContext(ouName);
        return UserPrincipal.FindByIdentity(principalContext, userName);
    }
}

Answer 1

I think static classes are nice in many cases. I could just say "use them when it's useful" but that's not very helpful.

My rule of thumb is (as always): Am I hiding any dependencies? If you feel like a "helper class" is useful (although beware of low cohesion) then by all means, go ahead. Just make sure your methods don't access global state though. Pure static methods are lovely. Static methods that depend on some globals or that open DB connections or read from disk/some config file are ticking time bombs.

They make testing the application really hard (the only way of doing it will be manually running the whole system, or with brittle automatic full-system tests, you'll get no granularity). They also make it impossible to swap out implementations.

Just remember the dependency inversion principle and the open/closed principle! Make sure your classes are pluggable. Make sure they don't pull stuff out of thin air. If you do that, then go ahead and make as many static methods as you want!

Answer 2

It looks like you are hiding an external dependency to Active Directory using this static class. One problem here is that if you are trying to unit test the class that calls these methods, you cannot fake static calls. So you immediately inhibit the testability of your code. I would refactor this to an interface, something like IProvideActiveDirectoryInformation, and concrete class, ActiveDirectoryInformationProvider. Then pass the interface in the constructor of your controller. This will allow you to wire up the concrete class with a DI container. It will also allow you to fake the ActiveDirectoryInformationProvider and return whatever you want for the interface methods.

I'd also look at not passing back things like the PrincipalSearcher object in the interface. If your method is GetAllUsersInOU() i would expect a list of users or usernames.

Answer 3

A class should be static if it only exists as an abstract concept in your application.

For example, say you're creating a clone of Twitter. You may have 2 types of tweets, user tweets and ads. They both share common behavior but are different. Thus, you want to use polymorphism and a factory to create one or another.

Those 2 tweets classes should be concrete classes, as they are real entities. Your domain is defined by these classes.

The factory should be static because it only exists at an abstract level to make your application better designed and to help you reuse code that'll create a type of tweet. Your domain isn't defined at any level by this factory.

So, if you don't think that a class should ever be instantiated but doesn't need to be extended to be used, it's probably a good sign that it should be static.

Answer 4

One of the things suggested in Object-Oriented Programming is Bad (the title is to get your attention, and the contents is disputed but entertaining) is that *Handler, *Manager and *Doer classes in general are a code smell that indicate someone trying to force object orientation onto a problem that is better suited for a procedural implementation.

In C# you can use static classes as module namespaces for procedures and preferably name them as such. I think this is how to do it, but not in your particular case.

Applications of your particular implementation will have behavioral dependencies of global state (say the permissions of a particular user), and as such should be an injected dependency so that the code that uses this can be unit tested in isolation.

Answer 5

I might be missing something but to my mind the answer boils down to the methods and member variables.

If these are all static, the class itself can (and should be) made static. If not, then it isn't a static class.

N.B. there is nothing that forces the class to be static even if the methods and variables are all static.

Answer 6

Consider the programming language you are using (C#) as a set of tools.

There is no rulebook, no "should" or "should not".

Look at the job are doing, and choose the best tool for the job.

If you need a global-available collection of methods, the same for all threads, then static classes are a good tool. If you want to store data within the class (different for each thread) then they are probably not.

Answer 7

There's nothing wrong in your Helper class. I have to confese I use this sort of Utils,Helpers or Holders to make accesible certain resources/logic from different places.

However an architect of my company tell me over and over that these classes often gather code that I don't know where to place at. He also dislikes because you could bring components from layers where upper/lower layers should not have access to and they performs tasks out of their responsabilities. It also brings some issues at test time.

I don't mind. I use'em and I use'em as properly as I think it should be used. And it works fine. So don't you worry.

It's true that at time of unit testing you will have a hard time mocking it up. But it will take just bit longer to do it. There're libs that goes through this issue.

Another approach could prevent to you from this sort of shortcomings (at testing). For instance, Singleton pattern. Injecting the instance as component dependency and turning methods to instance methods.

Anyways, so far I see, your solutions looks fine to me.