Using Static method and variables - Good vs Bad

Question

I am developing C# and asp.net web application.

I have general class called utilities, I have lot of public and static variables in this public utilities class.

Since this number is gradually increasing, I want to know is it good practice to store utilities methods and variable as public static.

Example of my code

public class utilities
{
    public static string utilVariable1 = "Myvalue";
    public static string utilVariable2 = "Myvalue";
    public static string utilVariable3 = "Myvalue";
    :
    public static string utilVariableN = "Myvalue";


    public static string UtilMethod1()
    {
         //do something
    }

    public static string UtilMethod2()
    {
         //do something
    }

    public static string UtilMethodN()
    {
         //do something
    }
}

Solution 1

There's nothing inherently wrong with static classes, although they should typically not have state (fields). Your use of public static fields indicates that this is not the case, so it seems like you are using abusing the static keyword slightly. If your class needs to have state, then it should be a normal, non-static class, and you should create instances of it. Otherwise, the only public fields visible on the class should be const (consider the Math class, with constants such as Math.PI - a good use of static methods and fields).

Another consideration is cohesion. Methods typically exist grouped in one class because they are closely related in one way or another. Again, the Math class is a good example; everything in there has to do with maths. At some point, you would want to split your global utility class into multiple smaller, more focussed ones. See Wikipedia for some examples on cohesion, it sounds like your usage falls under "Coincidental cohesion (worst)".

Solution 2

There's nothing wrong with this approach for methods, but variables should really be const if they're going to be static and public. If they are subject to change then you should look at a different structure for variables that are being manipulated by more than one component.

Personally, I'm a fan of the Singleton pattern.

Solution 3

static is not a bad thing per se. Methods that don't need to access any member variables or methods should always be declared static. That way the reader of the code sees immediately that a method won't change member variables or methods.

For variables the situation is different, you should avoid static variables unless you make them const. Public static variables are globally accessible and can easily raise issues if multiple threads access the same variable without proper synchronization.

It is hard to tell for your case if it's a good or a bad idea to use statics, because you didn't provide any context information.

Solution 4

Creating one class to do it all is not a good practice, and it's recommended to structure your project, and keep stuff that belongs to each other separated from the randomness.

A great example of this was a project I took over from a co-worker. There was 1 class, called Methods. It contained over 10K lines of methods.
I then categorized them into approx. 20 files, and the structure was restored.

Most of the methods from that project were validating user input, which can easily be moved into a static class Validation.

One awful thing I notice is the mutable public and static variables. This is bad for several reasons:

1. Incorrect behavior, because if some method changes this, while it isn't supposed to do that, it causes other methods to behave improperly, and it's really hard to track down/debug.
2. Concurrency, how are we going to ensure thread safety? Do we let it over to all methods that work with that? Say if it's a value type, what will we let them lock on? What if some method forgets to make it thread safe?
3. Expand-ability (I hope you understand what I mean with that), if you have for example a static class data that stores all these public static variables, that you shouldn't have. It can store that once, if for example you might change your application structure a bit, and say want to make it possible to load two projects in the same screen, then it's very difficult to make that possible, because you can't create two instances of a static class. There is only one class, and it'll remain like that.

For number 3 a cleaner solution would be to store either a list of instances of a data class, or to store a reference to the default and/or active data class.

Static member, and private static members (or protected) are a good practice, as long as you don't make huge classes, and the methods are related.

Public and static variables are okay if they're not really variable.
The two ways to do this is by marking them constant (const modifier) or readonly (readonly modifier).

Example:

public class UtilitiesClass
{
    internal UtilitiesClass() { }

    public void UtilityMethod1()
    {
        // Do something
    }
}

// Method 1 (readonly):
public static readonly UtilitiesClass Utilities = new UtilitiesClass();

// Method 2 (property):
private static UtilitiesClass _utilities = new UtilitiesClass();
public static UtilitiesClass Utilities
{
    get { return _utilities; }
    private set { _utilities = value; }
}

The advantage of method 1 is that you don't have to worry about thread-safety at all, the value can't change.
Method 2 is not thread-safe (though it's not difficult to make it that), but it has the advantage of allowing the static class itself to change the reference to the utilities class.

Solution 5

No, it is not a good practice for large applications, especially not if your static variables are mutable, as they are then effectively global variables, a code smell which Object Oriented Programming was supposed to "solve".

At the very least start by grouping your methods into smaller classes with associated functionality - the Util name indicates nothing about the purpose of your methods and smells of an incoherent class in itself.

Second, you should always consider if a method is better implemented as a (non-static) method on the same object where the data that is passed as argument(s) to the method lives.

Finally, if your application is quite large and/or complex, you can consider solutions such as an Inversion of Control container, which can reduce the dependency on global state. However, ASP.Net webforms is notoriously hard to integrate into such an environment, as the framework is very tightly coupled in itself.

View more solutions

Comments

[spender]

Yes, static methods without state are very useful, particularly when writing concurrent code. I have to disagree with you about Singleton though. I regard it as probably the most unnecessary of all patterns. Effectively it's global by another name and it's use discourages better structuring of code.

[Admin]

Can you please tell, whether it is good or bad approach, and if bad what is right approach by giving an example. Thank you

[Admin]

By utility class, method and variables i mean, that it is not associated with anything, its general in nature, Example: 1) to check whether string contains any bad words or not. 2) getting user ip

[Admin]

I have made that public, because, inorder to access it like UtilityClass.UtilityMethod1(); If possible for you can you please give me an example?

[Jonas Høgh]

I know it isn't always easy to group such methods. What I mean is that if your Util class contains 30 methods, and 10 of them are about, say, users, move those methods to another class that has "User" in its name.

[Aidiakapi]

I updated it with an example and some more text, is this what you were looking for?

[Admin]

Thank you @Aidiakapi, I appreciate your effort and help.