9

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);
    }
}
10
  • There's no mandate for avoiding static classes. Use them when it's useful to do so. There are good reasons the language designers included them. Jun 1, 2016 at 19:58
  • 5
    Why didn't you include these methods as static methods in their associated classes, instead of putting them all into a helper class? For example, your first method could be a static method in the PrincipalContext class, and be called like: PrincipalContext.Get(ouName). It's essentially a Factory method. Jun 1, 2016 at 20:01
  • @RobertHarvey I don't think I am following. This class is in my BLL and is called from many other classes (controllers) on the WebUI as well as a C# Console App that does nightly maintenance and to avoid having to recode it all the time I just wrote it into a helper class. Jun 1, 2016 at 20:04
  • 5
    Software development is always a series of tradeoffs. Which technique you choose to use depends on which set of tradeoffs you want to apply. Jun 1, 2016 at 20:14
  • 4
    @RobertHarvey: PrincipalContext and UserPrincipal are library classes, Jun 2, 2016 at 0:22

7 Answers 7

3

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!

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.

2
  • Not sure what you mean by "hiding an external dependency to Active Directory". I do make reads from AD and this class was built to reduce the coding needed to make connections and get basic data. Jun 2, 2016 at 19:42
  • that's what i mean. see Kai's answer for a fuller explanation. But what you do is no is make your code dependent on all the dependencies of the of the code your calling.
    – Fran
    Jun 2, 2016 at 20:01
1

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.

8
  • This would really depend on the type of factory. A simple static A BuildA() { return new A(); } sure, that's fine. but if you need anything more complex, such as when A has dependencies that need injecting or when different parts of your system needs different factory instances (abstract factory pattern) then a static method won't cut it.
    – sara
    Jun 2, 2016 at 7:11
  • Thanks for your comment. It doesn't sound like the factory you're talking about is so much abstract in the end, it looks like it is a concrete concept that needs to be instantiated (especially since you said you needed multiple different factories). Then again, nothing is set in stone anyway and only advices can be given about this subject. Jun 2, 2016 at 7:43
  • 1
    There exists many entities that is "Pure Fabrication", that doesn't in any way or form translate to the domain, e.g. UserDao. This is not an indication of whether or not it should be static though. It should be static if the units of work it does, does not depend on each other, and should be extendable, e.g. Math class. Jun 2, 2016 at 8:21
  • @ChrisWohlert very interesting comment (thumbed up), but doesn't it depend solely on the context ? I feel like an app about maths would want a Math class to be one of the core class and instantiable. Is UserDao some sort of repository between User and DB layer ? If so, why would you ever want to instantiate it ? You still could inject it even though you wouldn't ever create an object for this, since it doesn't "exist", what do you think ? Jun 2, 2016 at 9:43
  • 1
    @SteveChamaillard You have this assumption that it makes sense to instantiate only "real" objects. It reminds me these OOP 101 analogies where you have abstract class "Fruit" and inherited "Apple" - where OOP objects represent real world objects. But there's no need to limit yourself like this. OOP is a tool, so people use objects for "pure fabrications" even that makes their life easier and I see nothing wrong with it ...
    – qbd
    Jun 3, 2016 at 19:30
1

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.

0

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.

4
  • The same class doing the same things can be designed to be both static and non-static. There's always a design choice to be made.
    – sara
    Jun 2, 2016 at 9:58
  • There may be a design choice but the implementation is somewhat arbitrary. Nothing forces you to label the method or class static - even with a lack of an instance variable although some tools such as ReSharper do pick up on this...
    – Robbie Dee
    Jun 2, 2016 at 10:57
  • But when should I be making all the methods/members static or not? That is the question. When to use static and when not to. Jun 2, 2016 at 19:43
  • If none of the member variables or methods rely on an instance of the class - you have yourself a bona fide static class. By their nature, these tend to be things like helper classes or utilities and the like.
    – Robbie Dee
    Jun 2, 2016 at 20:04
0

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.

0

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.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.