Are Static Methods an Anti-Pattern?

Is using the Static keyword on class members and methods an anti-pattern? I think so and hopefully this post will get you thinking about it. Primarily I see Static abused when programmers do not want to go through the work of instantiating a class. This is in circumstances in which instantiation is not strictly necessary. The problem with Static members is that they get in the way of approaches such as Inversion of Control and Test Driven development.

First, lets take a look at an example that illustrates what I am talking about concerning using Static on class members. For the sake of discussion I will be using java, but the same is true in c#. Here is the main object of discussion, a Sentinel. This implementation uses an object with Static methods as a dependent object.

public class StaticSentinel {
    private AiState aiState = AiState.Scanning;
    private long posX;
    private long posY;
    private long posZ;

    public void live() {
        int threatLevel = 0;

        // unit testing here is problematic because of the dependency upon StaticMutantScanner. Since
        // StaticMutantScanner's implementation is static it cannot be overriden. Therefore, this.live() cannot
        // be unit tested in isolation of the dependency upon StaticMutantScanner. A unit test here is no longer
        // a unit test but is an integration test - testing both StaticSentinel and StaticMutantScanner components.
        Mutant mutant = StaticMutantScanner.detectMutant(this.posX, this.posY, this.posZ);

        if (mutant != null) {
            threatLevel = StaticMutantScanner.assessThreatLevel(mutant);
            // do something here based on the threat level.
            if (threatLevel > 0) {
                this.aiState = aiState.Hunting;
            }
        }

        // some other ai related activities, move, sleep, attack, guard, etc.
        doAi();
    }

    private void doAi() {
        // do something here.
    }

    public AiState getAiState() {
        return this.aiState;
    }
}

And here is the class with the Static methods. Our scanner, it looks for Mutants and assess their threat level.

public class StaticMutantScanner {
    public static Mutant detectMutant(long posx, long posy, long posz) {
        // create a mutant here via factory pattern and return it if nearby
        // instead of newing-up this mutant. newing-up is used here just for illustration.
        return new Mutant();
    }

    public static int assessThreatLevel(Mutant mutant) {
        // do something complicated here
        // and return threat level of the mutant
        return 0;
    }
}

What I’ve outlined above in the Sentinel class is not an uncommon thing to see in legacy code bases. In the case of the StaticMutantScanner - it is an object that does not require instantiation. So, the developer creates some kind of Static implementation. It makes sense, why bother instantiating it and all the irritating work associated with the task? This thought works - until we try to test it.

If we don’t care about good software quality and highly testable software, we don’t really need to care that we have a Static member. Some organizations out there still test by releasing or through manual and labor intensive processes. For the rest of us though, we want to know quickly and reliably - if and when we break our software by making inevitable changes. So we test. For us, Unit Testing is not an academic exercise.

Here is the test of the StaticSentintel:

    @Test
    public void liveTest(){
        StaticSentinel sentinel = new StaticSentinel();
        sentinel.live();
        Assert.assertTrue(sentinel.getAiState() == AiState.Scanning);
    }

Unit Testing is where Static methods and members make things go wrong. If I try to Unit Test Sentinel.live() in its current state, I cannot do so without exercising the code that lives in StaticMutantScanner.detectMutant() and StaticMutantScanner.accessThreat(). That is, I test live() by settings the a Sentinel’s coordinates and based on whether or not a Mutant is nearby get some kind of AiState property value to Assert against. So this means I have to guess or know in advance the values that would make StaticMutantScanner behave the way we need it to to cover bother branches of the “ if (threatLevel > 0) {“ statement. Some of you at this point are saying, ‘yes, that is Unit Testing.’

No it isn’t. It is an Integration Testing of the Sentinel and MutantScanner objects and how they work in tandem. In a Unit Test - a test of Sentinel.live() should be done by not actually descending into the the logic exposed by sub-components; thereby ‘testing the smallest unit of code’, a.k.a., Unit Test. Integration Testing is perfectly valid and should be done along side Unit Testing, but this is probably the job a QA person, not a Dev. Regardless of the ‘who’ both Integration and Unit tests should exist and serve distinctly different purposes.

In this scenario, Inversion of Control (IoC), also known as Dependency Injection (DI), becomes extremely important and is one reason why DI has emerged as a best practice. In the case of Unit Testing, DI lets us inject an implementation of a dependent child object to explicitly cause a state to exist in the child component that will cause one branch of code to behave in an expected manner.

For this discussion I implement DI this through an overloaded constructor and without some kind of IoC container. I have created a new Sentinel object named TestableSentinel. This object has an overloaded constructor and now acts more like a true composite object with an instantiated MutantScanner member. Note that the calls to the MutantScanner Interface are now through an instantiated object, not a class with Static members.

public class TestableSentinel {
    private MutantScanner mutantScanner;
    private AiState aiState = AiState.Scanning;
    private long posX;
    private long posY;
    private long posZ;

    /**
     * Overloaded constructor. Ultimately for use with Inversion of Control/Dependency Injection.
     * We can inject an implementation of MutantScanner at run time to properly Unit Test
     * TestableSentinel
     *
     * @param mutantScanner This is an implementation of an interface.
     */
    public TestableSentinel(MutantScanner mutantScanner){
        this.mutantScanner = mutantScanner;
    }

    public void live() {
        int threatLevel = 0;
        // The problem with the dependency upon mutantScanner's implementations are solved here by passing in
        // a new implementation during testing that returns the expected result for each test. This ensures that
        // TestableSentinel.live() is being tested in isolation of the mutantScanner object.
        Mutant mutant = this.mutantScanner.detectMutant(this.posX, this.posY, this.posZ);

        if (mutant != null) {
            threatLevel = this.mutantScanner.assessThreatLevel(mutant);
            // do something here based on the threat level.
            if (threatLevel > 0) {
                this.aiState = aiState.Hunting;
            }
        }
        // some other ai related activities, move, sleep, attack, guard, etc.
        doAi();
    }

    private void doAi() {
        // do something here.
    }

    public AiState getAiState() {
        return this.aiState;
    }

Here is the MutantScanner Interface:

public interface MutantScanner {
    Mutant detectMutant(long posx, long posy, long posz);
    int assessThreatLevel(Mutant mutant);
}

Note that we don’t know what the implementation of MutantScanner does. We just know that if it returns a Mutant and returns a threatLevel > 0 that the Sentinel will start Hunting. This is all we need to know and should know for the purpose of Unit Testing TestableSentinel.live().

The tests are a bit more interesting. I’ve deliberately not supplied an implementation of MutantScanner in the main branch of the source. This is to illustrate that one of the purposes behind the DI in the overloaded constructor of TestableSentinel is that we can supply an implementation specific to our test’s needs.

Here is the first test:

    @Test
    public void liveTest_ExpectThreatLevelHunting(){
        int threatLevel = 1;
        MutantScanner mutantScanner = new TestMutantScanner(threatLevel);
        TestableSentinel sentinel = new TestableSentinel(mutantScanner);
        sentinel.live();
        Assert.assertTrue(sentinel.getAiState() == AiState.Hunting);
    }

Here we set the threatLevel as a parameter in the TestMutantScanner’s constructor. TestMutantScanner is an implemetnation of MutantScanner created solely for the purpose of Unit Testing the Sentinel’s live() method. In this implementation, assessThreatLevel() simply returns the value supplied to the constructor when the TestMutantScanner is instantiated. When the test is run, the branch of code is executed that sets the sentinel’s AiState to Hunting and the test verifies this.

public class TestMutantScanner implements MutantScanner {
    int threatLevel;

    /**
     * For the purpose of discussion here and the associated test,
     * this implementation simply returns the threatLevel that is set in the constructor.
     * This is to give a point in which I can control the branch of code to unit test in
     * TestableSentinel via this implementation.
     * @param threatLevel
     */
    public TestMutantScanner(int threatLevel){
        this.threatLevel = threatLevel;
    }
    public Mutant detectMutant(long posx, long posy, long posz){
        return new Mutant();
    }

    public int assessThreatLevel(Mutant mutant){
        return threatLevel;
    }
}

We test the other branch, the branch that sets the Sentinel’s AiMode to Scanning, just by changing the parameter passed to TestMutantScanner to 0. This gets complete coverage of both branches of code associated with the if (threatLevel > 0) line of code. See the second test:

    @Test
    public void liveTest_ExpectThreatLevelScanning(){
        int threatLevel = 0;
        MutantScanner mutantScanner = new TestMutantScanner(threatLevel);
        TestableSentinel sentinel = new TestableSentinel(mutantScanner);
        sentinel.live();
        Assert.assertTrue(sentinel.getAiState() == AiState.Scanning);
    }

That’s how we get great Unit Test coverage. Unit Testing takes work, but it reduces delivered bugs. It also reveals flaws in design as in the case of Static methods. Some tools and frameworks exist to make this easier - MOQ http://code.google.com/p/moq/in the .Net site of things and Mockito http://code.google.com/p/mockito/ on the java side of things are two examples. The topic of mocking gets pretty big. Essentially, I created a manual mock when I implemented MutantScanner for the purpose of testing.

To reiterate - the problem with Static members lie in inability to test them. We can’t change the implementation. This creates untestable software when the Static members are in a class that is used by another. None of the testing strategies outlined above work for static methods.

Other strategies besides DI/IoC exist for dealing with Static methods, such as wrapper classes. But if we can, we should refactor and eliminate these legacy artifacts. We certainly should not inject Static methods into new code bases without compelling reasons to do so. Using Test Driven Development will reveal the pain associated with Static methods and the need to avoid them.