Refactoring a static class to use with dependency injection

Question

We need to use an unmanaged library in our code that has static methods. I'd like to introduce the library operation as a dependency in my code. And apart from having static methods, the library has an initialization method and a settings method, both are global. So I can't just wrap this in an instance class, because if one instance changes a setting, all other instances will be affected, and if one instance gets initialized, all other instances will be reinitialized.

I thought about introducing it as a singleton class. This way it will be in an instance class, but there will only be one instance thus I won't have to worry about changing the settings or initialization. What do you think about this approach? I'm pretty new to the dependency injection pattern and I'm not sure if the singleton pattern is a good solution? What would your solution be to a similar case?

Edit: The initialization takes a parameter too, so I can't just lock the method calls and re-initialize and change settings every time it is called.

Edit 2: Here are the signatures of some methods:

public static void Initialize(int someParameter)
// Parameter can only be changed by re-initalization which
// will reset all the settings back to their default values.

public static float[] Method1(int someNumber, float[] someArray)

public static void ChangeSetting(string settingName, int settingValue)

Answer 1

If you only need to set the settings once at start up, then I would recommend making a non-static wrapper class which does all the initialization of the static class in its own static constructor. That way you can be assured that it will only happen once:

public class MyWrapper
{
    public MyWrapper()
    {
        // Do any necessary instance initialization here
    }

    static MyWrapper()
    {
        UnManagedStaticClass.Initialize();
        UnManagedStaticClass.Settings = ...;
    }

    public void Method1()
    {
        UnManagedStaticClass.Method1();
    }
}

However, if you need to change the settings each time you call it, and you want to make your instances thread-safe, then I would recommend locking on a static object so that you don't accidentally overwrite the static settings while they're still in use by another thread:

public class MyWrapper
{
    public MyWrapper()
    {
        // Do any necessary instance initialization here
    }

    static MyWrapper()
    {
        UnManagedStaticClass.Initialize();
    }

    static object lockRoot = new Object();

    public void Method1()
    {
        lock (lockRoot)
        {
            UnManagedStaticClass.Settings = ...;
            UnManagedStaticClass.Method1();
        }
    }
}   

If you need to pass initialization parameters into your class's instance constructor, then you could do that too by having a static flag field:

public class MyWrapper
{
    public MyWrapper(InitParameters p)
    {
        lock (lockRoot)
        {
            if (!initialized)
            {
                UnManagedStaticClass.Initialize(p);
                initialized = true;
            }
        }
    }

    static bool initialized = false;
    static object lockRoot = new Object();

    public void Method1()
    {
        lock (lockRoot)
        {
            UnManagedStaticClass.Settings = ...;
            UnManagedStaticClass.Method1();
        }
    }
}

If you also need to re-initialize each time, but you are concerned about performance because re-initializing is too slow, then the only other option (outside of the dreaded singleton) is to auto-detect if you need to re-initialize and only do it when necessary. At least then, the only time it will happen is when two threads are using two different instances at the same time. You could do it like this:

public class MyWrapper
{
    public MyWrapper(InitParameters initParameters, Settings settings)
    {
        this.initParameters = initParameters;
        this.settings = settings;
    }

    private InitParameters initParameters;
    private Settings settings;
    static MyWrapper currentOwnerInstance;
    static object lockRoot = new Object();

    private void InitializeIfNecessary()
    {
        if (currentOwnerInstance != this)
        {
            currentOwnerInstance = this;
            UnManagedStaticClass.Initialize(initParameters);
            UnManagedStaticClass.Settings = settings;
        }
    }

    public void Method1()
    {
        lock (lockRoot)
        {
            InitializeIfNecessary();
            UnManagedStaticClass.Method1();
        }
    }
}

Answer 2

I would use a stateless service class, and pass in state info for the static class with each method call. Without knowing any details of you class, I'll just show another example of this with a c# static class.

public static class LegacyCode
{
    public static void Initialize(int p1, string p2)
    {
        //some static state
    }
    public static void ChangeSettings(bool p3, double p4)
    {
        //some static state
    }
    public static void DoSomething(string someOtherParam)
    {
        //execute based on some static state
    }
}

public class LegacyCodeFacadeService
{
    public void PerformLegacyCodeActivity(LegacyCodeState state, LegacyCodeParams legacyParams)
    {
        lock (_lockObject)
        {
            LegacyCode.Initialize(state.P1, state.P2);
            LegacyCode.ChangeSettings(state.P3, state.P4);
            LegacyCode.DoSomething(legacyParams.SomeOtherParam);
            //do something to reset state, perhaps
        }
    }
}

You'll have to fill in the blanks a little bit, but hopefully you get the idea. The point is to set state on the static object for the minimum amount of time needed, and lock access to it that entire time, so no other callers can be affected by your global state change. You must create new instances of this class to use it, so it is fully injectable and testable (except the step of extracting an interface, which I skipped for brevity).

There are a lot of options in implementation here. For example, if you have to change LegacyCodeState a lot, but only to a small number of specific states, you could have overloads that do the work of managing those states.

EDIT

This is preferable to a singleton in a lot of ways, most importantly that you won't be able to accumulate and couple to global state: this turns global state in to non-global state if it is the only entry point to your static class. However, in case you do end up needing a singleton, you can make it easy to switch by encapsulating the constructor here.

public class LegacyCodeFacadeService
{
    private LegacyCodeFacadeService() { }

    public static LegacyCodeFacadeService GetInstance()
    {
        //now we can change lifestyle management strategies later, if needed
        return new LegacyCodeFacadeService();
    }

    public void PerformLegacyCodeActivity(LegacyCodeState state, LegacyCodeParams legacyParams)
    {
        lock (_lockObject)
        {
            LegacyCode.Initialize(state.P1, state.P2);
            LegacyCode.ChangeSettings(state.P3, state.P4);
            LegacyCode.DoSomething(legacyParams.SomeOtherParam);
            //do something to reset state, perhaps
        }
    }
}