Refactoring classes that use global variables.

Question

I'm working on some classes that get part of their configuration from global variables, e.g.

class MyClass {
    public void MyClass(Hashtable<String, String> params) {
        this.foo = GlobalClass.GLOBALVAR.get("foo");
        this.bar = GlobalClass.GLOBALVAR.get("bar");
        this.params = params;
    }
}

This is bad for a couple of reasons, GLOBALVAR talks to a database to get some of the variables and this makes it really hard to make unit tests. The other problem is that I have many (dozens) of classes that inherit from MyClass, so I can't easily change the constructor signature.

My current solution is to create an additional default constructor and setter methods for params, foo and bar.

class MyClass {
     // Other code still here for backwards compatibility.
     public void MyClass() {
         // Do nothing much.
     }
     public void setParams(Hashtable<String, String> params) {
         this.params = params;
     }
     public void setFoo(Foo foo) {
         this.foo = foo;
     }
     public void setBar(Bar bar) {
         this.bar = bar;
     }
}

Any ideas on a good way to refactor this, besides the way I did it? My other thought would be to use a factory method, but I'm afraid I'll run into polymorphic substitution problems.

Answer 1

A slight variation on your approach would be to have an object of type GLOBALVAR in the class and use that instead of the actual global (that refactoring should be a simple search/replace). You can default the new variable to the actual global variable and provide an override for testing.

Answer 2

Your class should take all of its dependencies in the constructor. It's a good idea to make it impossible to create an invalid or uninitialized instance of classes. Make foo and bar private and final, and set them in the constructor.

Answer 3

I think you should introduce an interface to put a layer of abstraction between the global variable collection and its consumers.

interface GlobalVars {
   String get(String key);
}

You should introduce a constructor with limited scope, probably package-private

MyClass(GlobalVars globals, Map<String, String> params) {
   // create the object
}

And then provide public static factory methods to use this constructor.

public static MyClass newMyClass(Map<String, String> params) {
   return new MyClass(GlobalClass.GLOBAL_VAR, params);
}

With this design you can pass in a mock implementation of GlobalVars in a unit test from within the same package by explicitly invoking the constructor.

Addendum: Since params seems to be a required field, I would definitely make it final and avoid the approach where you add mutators to overwrite them.

private final Map<String, String> params;

Also, make a defensive copy to prevent l33t h4x.

this.params = Collections.unmodifiableMap(params);

Answer 4

This GlobalClass.GLOBALVAR should be chopped up up into logical units. That way it would be easier to make mock objects for the unit tests. For example in my CAD/CAM metal cutting application I have a MaterialList, a SheetSizeList, PartNestingParameters, etc.

I don't have a huge list of variables stuff into one giant AppParameter class. They all hang off a ShopStandards object. For Unit Test involving a specific PartNestingParmeters I will just go ShopStandards.PartNestingParmeters = new MockPartNestingParameterTest114(). The test will run not realizing that the Part Nesting Parameters are a mockup. Plus this save me from having to doing dozens of assignments just to get the ShopStandard setup correctly for the test.

We have even more automated where many of the Mock load from files saved during the test run during initial development.

Answer 5

I think I would start by doing the following. It let's your existing code work without modification, and allows you to add new constructors to the subclasses as you can. Once all of the subclasses have the new constructor, and all of the calls to the old constructors are gone, you can get rid of the GlobalClass and the constructors that use it. You can also then, hopefully, work on cleaning up the GLOBALVAR (the Car class in my code).

import java.util.Hashtable;


class MyClass
{
    private final Foo foo;
    private final Bar bar;
    private final Hashtable<String, String> params;

    public MyClass(final Hashtable<String, String> params)
    {
        this(params, GlobalClass.GLOBALVAR);
    }

    // added constructor
    public MyClass(final Hashtable<String, String> params, 
                   final FooBar fooBar)
    {
        this.foo    = fooBar.getFoo();
        this.bar    = fooBar.getBar();
        this.params = params;
    }
}

class MySubClass
    extends MyClass
{
    public MySubClass(final Hashtable<String, String> params)
    {
        super(params);
    }

    // added constructor
    public MySubClass(final Hashtable<String, String> params, 
                      final FooBar fooBar)
    {
        super(params, fooBar);
    }
}

// unchanged
class GlobalClass
{
    public static Car GLOBALVAR;
}

// added interface
interface FooBar
{
    Foo getFoo();
    Bar getBar();
}

class Car
    // added implements
    implements FooBar
{
    private Foo foo = new Foo();
    private Bar bar = new Bar();

    public Object get(final String name)
    {
        if(name.equals("foo"))
        {
            return (foo);
        }

        if(name.equals("bar"))
        {
            return (bar);
        }

        throw new Error();
    }

    // added method
    public Foo getFoo()
    {
        return ((Foo)get("foo"));
    }

    // added method
    public Bar getBar()
    {
        return ((Bar)get("bar"));
    }
}

// unchanged
class Foo
{
}

// unchanged
class Bar
{
}

Answer 6

Since you mention that you have the freedom to modify the class hierarchy.

• Change the base MyClass ctor to take in 3 parameters params, foo and bar. Comment out the GlobalVar references and simply cache passed in values
• Compile.. this should throw up a bunch of compile errors - no ctor which takes 1 parameter.
• Fix each one to pass in GlobalVar.get("foo") and GlobalVar.get("bar"). Get it to build.
• Refine: Now minimize hits to the DB by lazy load and caching the foo and bar values. Expose via some property on GlobalVar.