Preventing inadvertent changes to static classes

This is a followup to these posts, where I developed a way of opening up a seam along filesystem access in C#.

The last post highlighted a problem with the approach: when behaviour is changed by injecting a new delegate it is changed for all subsequent users of the class.

This is a small problem in tests – you just need to make sure that you reset your static class state in test setup and tear down.

In a production system however it could be a much larger problem. A developer could change the behaviour of the class without realising the consequences, and change the behaviour of the system in an unexpected way. Equally, they could deliberately subvert the behaviour of the system – but if they’re going to do that, they’d probably find a better way of doing it.

You could avoid this by treating this technique as only a short term measure to open a seam while refactoring legacy code, and rapidly refactor again to something more robust, perhaps using a factory or IOC container to inject different implementations for test or production code. This is entirely reasonable; the technique gets very quick and easy results, and you could definitely use it while characterizing legacy code and rapidly refactor away from it.

Alternatively you could make it somewhat lockable, and have it warn if code tries to change behaviour without. I’m musing now, because I tend to think about this technique in the sense I’ve just described. In principle though you could guard against careless changes in behaviour by having the class “locked” by default, and only allow changes to behaviour after first unlocking the class.

Tangentially this raises a question – is a physical lock a form of security by obfuscation and awkwardness?

To give an example, consider this test code for a notional File class:

    [TestFixture]
    public class FileTests
    {
        [SetUp]
        public void SetUp()
        {
            File.Unlock();
            File.CopyImpl = null;
            File.Lock();
        }

        [Test]
        [ExpectedException("System.InvalidOperationException")]
        public void File_ShouldWarn_IfBehaviourChangesWhenLocked()
        {
            File.CopyImpl = null;
        }

        [Test]
        public void File_ShouldAllow_BehaviourChangeWhenUnlocked()
        {
            File.Unlock();
            File.CopyImpl = (src, dst, ovrt) => { };
        }
    }

I don't see the lock as a strong lock -- just a flag to make a developer stop and think about what they're doing, and to allow a mechanism to raise an alert if it's done without thought. An implementation that matches these tests could look like:

    public static class File
    {
        private static Action<string, string, bool> _copyImpl = System.IO.File.Copy;

        private static bool _locked = true;

        public static Action<string, string, bool> CopyImpl
        {
            get
            {
                return _copyImpl;
            }
            set
            {
                if(_locked)
                {
                    throw new InvalidOperationException("IO.Abstractions.File is locked to disallow behaviour changes");
                }
                _copyImpl = value;
            }
        }

        public static void Unlock()
        {
            Trace.WriteLine("IO.Abstractions.File now unlocked for behaviour changes");
            _locked = false;
        }

        public static void Lock()
        {
            Trace.WriteLine("IO.Abstractions.File now locked for behaviour changes");
            _locked = true;            
        }

        public static void Copy(string source, string destination, bool overwrite)
        {
            CopyImpl(source, destination, overwrite);
        }
    }

The underlying implementation is the same as I presented before, with a couple of changes. There are now obvious Lock and Unlock methods; I'm using these to just set or unset an internal flag. I'm wrapping the point of injection of new behaviour for CopyImpl in a property, and using the set method to check the locking status. If the class is locked, it'll throw an exception to indicate that you're not meant to be changing it. The class is locked by default.

Exactly how useful this is in the long term I'm not sure. It's tempting to keep the Abstractions in place, as the class structure is simpler than the normal interface-and-factory style approach that you might use out of choice. It's possible that an IOC container might be abused in the same way as this technique, by a developer who wants a specific local behaviour but doesn't understand that their changes might have global consequences.

And I've not even touched on any possible threading problems.