9

I was recently reviewing a few Helper-style "utility bag" static classes floating around some large C# codebases I work with, things basically like the following very condensed snippet:

// Helpers.cs

public static class Helpers
{
    public static void DoSomething() {}
    public static void DoSomethingElse() {}
}

The specific methods I've reviewed are

  • mostly unrelated to each other,
  • without explicit state persisted across invocations,
  • small, and
  • each consumed by a variety of unrelated types.

Edit: The above is not intended to be a list of alleged problems. It's a list of common characteristics of the specific methods I'm reviewing. It's context to help answers provide more relevant solutions.

Just for this question, I'll refer to this kind of method as a GLUM (general lightweight utility method). The negative connotation of "glum" is partly intended. I'm sorry if this comes across as a dumb pun.

Even putting aside my own default skepticism about GLUMs, I don't like the following things about this:

  • A static class is being used solely as a namespace.
  • The static class identifier is basically meaningless.
  • When a new GLUM gets added, either (a) this "bag" class gets touched for no good reason or (b) a new "bag" class gets created (which in itself is not usually an issue; what's bad is that the new static classes often just repeat the unrelatedness problem, but with fewer methods).
  • The meta-naming is inescapably awful, non-standard, and usually internally inconsistent, whether it's Helpers, Utilities, or whatever.

What's a reasonably good & simple pattern for refactoring this, preferably addressing the above concerns, and preferably with as light a touch as possible?

I should probably emphasize: All the methods I'm dealing with are pairwise unrelated to each other. There doesn't seem to be a reasonable way to break them down into finer-grained yet still multi-member static class method-bags.

|improve this question
  • 1
    About GLUMs™: I think abbreviation for "lightweight" could be removed, since methods should be lightweight by following best practices ;) – Fabio Oct 8 '17 at 9:50
  • @Fabio I agree. I included the term as a (probably not terribly funny and possibly confusing) joke that serves as an abbreviation for this question's write-up. I think "lightweight" seemed appropriate here because the codebases in question are long-lived, legacy, and a little messy, i.e. there happen to be many other methods (not usually these GUMs ;-) that are "heavyweight". If I had more (ehem, any) budget at the moment, I'd definitely take a more aggressive approach and address the latter. – William Oct 8 '17 at 17:04
17

I think you have two problems closely related to each other.

  • Static classes contains logically unrelated functions
  • Names of static classes do not provide helpful information about meaning/context of the contained functions.

These problems can be fixed by combining only logically related methods into one static class and moving the others into their own static class. Based on your problem domain, you and your team should decide what criteria you will use for separating methods into related static classes.

Concerns and possible solutions

Methods are mostly unrelated to each other - problem. Combine related methods under one static class and move unrelated methods to their own static classes.

Methods are without explicit state persisted across invocations - not a problem. Stateless methods are a feature, not a bug. Static state is difficult to test anyway, and should only be used if you need to maintain state across method invocations, but there are better ways to do that such as state machines and the yield keyword.

Methods are small - not a problem. - Methods should be small.

Methods are each consumed by a variety of unrelated types - not a problem. You have created a general method which can be re-used in many not related places.

A static class is being used solely as a namespace - not a problem. This is how static methods are used. If this style of calling methods bothers you, try using extension methods or the using static declaration.

The static class identifier is basically meaningless - problem. It is meaningless because developers are putting unrelated methods in it. Put unrelated methods into their own static classes and give them specific and understandable names.

When a new GLUM gets added, either (a) this "bag" class gets touched (SRP sadness) - not a problem if you follow idea that only logically related methods should be in one static class. SRP is not violated here, because principle should be applied for classes or for methods. Single responsibility of static classes means to contain different methods for one general "idea". That idea can be a feature or a conversion, or iterations(LINQ), or data normalization, or validation ...

or less frequently (b) a new "bag" class gets created (more bags) - not a problem. Classes/static classes/functions - are our(developers) tools, which we use for designing software. Does your team have some limits for using classes? What are maximum limits for "more bags"? Programming is all about context, if for solution in your particular context you end up with 200 static classes which understandably named, logically placed in namespaces/folders with logical hierarchy - it is not a problem.

The meta-naming is inescapably awful, non-standard, and usually internally inconsistent, whether it's Helpers, Utilities, or whatever - problem. Provide better names that describe expected behaviour. Keep only related methods in one class, which provide help in better naming.

|improve this answer
  • It’s not a SRP violation, but pretty clearly an Open/Closed Principle violation. That said, I’ve generally found the Ocp to be more trouble than it’s worth – Paul Oct 8 '17 at 5:21
  • 2
    @Paul, Open/Close principle can not be applied for static classes. That was my point, that SRP or OCP principles can not be applied for static classes. You can apply it for static methods. – Fabio Oct 8 '17 at 5:53
  • Okay, there was some confusion here. The question's first list of items is not a list of alleged problems. It's a list of common characteristics of the specific methods I'm reviewing. It's context to help answers provide more applicable solutions. I'll edit to clarify. – William Oct 8 '17 at 17:08
  • 1
    Re "static class as namespace", the fact it's a common pattern doesn't mean it's not an anti-pattern. The method (which is basically a "free function" to borrow a term from C/C++) within this particular low-cohesion static class is the meaningful entity in this case. In this particular context, it seems structurally better to have a sub-namespace A with 100 single-method static classes (in 100 files) than a static class A with 100 methods in a single file. – William Oct 8 '17 at 17:36
  • 1
    I did some fairly major cleanup on your answer, mostly cosmetic. I'm not sure what your last paragraph is about; nobody worries about how they are testing code that calls the Math static class. – Robert Harvey♦ Oct 10 '17 at 21:14
4

Just embrace the convention that static classes are used like namespaces in situations like this in C#. Namespaces get good names, so should these classes. The using static feature of C# 6 makes life a little easier, too.

Smelly class names like Helpers signal that the methods should be split up into better-named static classes, if at all possible, but one of your emphasized assumptions is that the methods you're dealing with are "pairwise unrelated", which implies splitting down to one new static class per existing method. That's probably fine, if

  • there are good names for the new static classes and
  • you judge it to actually benefit your project's maintainability.

As a contrived example, Helpers.LoadImage might become something like FileSystemInteractions.LoadImage.

Still, you could end up with static class method-bags. Some ways this can happen:

  • Maybe only some (or none) of the original methods have good names for their new classes.
  • Maybe the new classes later evolve to include other relevant methods.
  • Maybe the original class name wasn't smelly and was actually okay.

It's important to remember that these static class method-bags are not terribly uncommon in real C# codebases. It's probably not pathological to have a few small ones.

If you really see an advantage in having each "free function"-like method in its own file (which is fine and worthwhile if you honestly judge that it actually benefits your project's maintainability), you could consider making such a static class instead a static partial class, employing the static class name similar to how you would a namespace and then consuming it via using static. For example:

structure of dummy project using this pattern

Program.cs

namespace ConsoleApp1
{
    using static Foo;
    using static Flob;

    class Program
    {
        static void Main(string[] args)
        {
            Bar();
            Baz();
            Wibble();
            Wobble();
        }
    }
}

Foo/Bar.cs

namespace ConsoleApp1
{
    static partial class Foo
    {
        public static void Bar() { }
    }
}

Foo/Baz.cs

namespace ConsoleApp1
{
    static partial class Foo
    {
        public static void Baz() { }
    }
}

Flob/Wibble.cs

namespace ConsoleApp1
{
    static partial class Flob
    {
        public static void Wibble() { }
    }
}

Flob/Wobble.cs

namespace ConsoleApp1
{
    static partial class Flob
    {
        public static void Wobble() { }
    }
}
|improve this answer
-5

Generally you shouldn't have or require any "general utility methods". In your business logic. Take another hard look and put them in a suitable place.

If you are writing a maths library or something then I would suggest, although I normally hate them, Extension Methods.

You can use Extension Methods to add functions onto standard datatypes.

For example Lets say I have a general function MySort() instead of Helpers.MySort or adding a new ListOfMyThings.MySort I can add it to IEnumerable

|improve this answer
  • The point is to not take yet another "hard look". The point is to have a reusable pattern that can easily clean up a "bag" of utilities. I know that utilities are smells. The question states this. The goal is to sensibly isolate these independent (but too closely co-located) domain entities for now and go as easy as possible on the project budget. – William Oct 8 '17 at 19:34
  • 1
    If you don't have "general utility methods," you're probably not isolating pieces of logic from the rest of your logic enough. E.g., as far as I know, there's no general purpose URL path joining function in .NET, so I often end up writing one. That kind of cruft would better off as part of the standard library, but every standard lib is missing something. The alternative, leaving the logic repeated directly inside your other code, makes it vastly less readable. – jpmc26 Oct 11 '17 at 23:32
  • stackoverflow.com/questions/372865/path-combine-for-urls – Ewan Oct 12 '17 at 5:12
  • 1
    @Ewan that is not a function, it is a general purpose delegate signature... – TheCatWhisperer Oct 12 '17 at 21:01
  • 1
    @Ewan That was TheCatWhisperer's entire point: utility classes are a kludge around having to put methods in a class. Glad you agree. – jpmc26 Oct 24 '17 at 4:16

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.