Requring static class setter to be called before constructor, bad design?

Question

I have a class, say Foo, and every instance of Foo will need and contain the same List object, myList.

Since every class instance will share the same List Object, I thought it would be good to make myList static and use a static function to set myList before the constructor is called.

I was wondering if this was bad, because this requires the setter to be called before the constructor? If the person doesn't, the program will crash. Alternative way would be passing myList every time.

Answer 1

Requiring a certain method to be called before another is called Sequential Coupling, and is often considered to be an antipattern. Take for example this code generator:

cg.enterScope();
... // add operations inside this scope
cg.leaveScope();

Here, leaveScope always has to be called after enterScope, else incorrect code is emitted. This can be trivially refactored to use a Scope type:

Scope(scoped_operations).generate_code(cg);

Your case is trickier, as you want to lazily initialize a static member before any instances of that class are created. I guess this will look like

TheClass.initialize();
var instance = new TheClass();  // first use

The first step is to make initialize threadsafe. Then, it has to record whether it has already run. If so, any future calls simply short circuit:

static bool is_initialized = false;

static synchronized void initialize() {
  if (is_initialized) return;
  ...; // normal initialization
  is_initialized = true;
}

Now we can do this call inside the constructor to make sure the class is always initialized before other code runs:

TheClass(...) {
  if (!is_initialized) initialize(); // try to avoid call to synchronized method
  ...;
}

---

If the initialization doesn't have to happen lazily, various simpler solutions can be used. (In this context, lazy initialization means that initialization is only ever performed if an instance is created). For something as cheap as creating a List object, you will probably want to initialize it regardless of any objects created. Different languages have different mechanisms, e.g.

// Java, C#
static List<Foo> myList = new List<Foo>();

// Java: static initializer block
static {
  myList = new List<Foo>();
}

---

If the initialization value is not known at compile time, but is rather provided by a user, this gets complicated again. In some languages you can parameterize classes themselves, e.g. TheClass = new TheMetaClass(42); instance = new TheClass(). In languages lacking a Metaobject Protocol, the Factory Pattern can be used instead: factory = new TheClassFactory(42); instance = factory.create(). This looks similar to the sequentially coupled example at the beginning, but with a factory the type system can guarantee that initialization has happened before an instance is created.

Answer 2

Yes, it's bad.

Consider the alternative where you always pass in the list in constructor; you can still make each object receive the same list, e.g. by storing the list in the factory for the object, and passing it on new.

And since the class advertises that it needs a list instance by only offering a constructor that takes this instance, the user knows he's passing in null if he's not using the factory. This is much more obvious than requiring to call a static function.

Answer 3

In C# you can create the list instance in the static constructor implicitly:

static List<int> myList = new List<int>();

public Foo()
{
   myList.Add(42); // note: not thread safe!!!
}

or explicitly:

static List<int> myList;

static Foo()
{
   myList = new List<int>();    
}

public Foo()
{
   myList.Add(42); // note: not thread safe!!!
}

This is better than setting it independently for a number of reasons, but mostly:

• static constructors only execute once (useful in multithreaded contexts)
• static constructors are run before anything else, so you can assume that the field is always initialized

More in general, it is good practice to make a class "ready to use" in the constructor, and static members in the static constructor. This prevents having an instance with uninitialized fields.

Requiring a particular order of calls is possibly an anti-pattern known as Sequential Coupling and I would avoid doing that unless necessary.

Answer 4

By having global state you effectively implemented a FooFactory as a singleton (well, using static fields, but the effect is the same).

How about making the factory part explicit and getting rid of the singleton part (as singleton is quite untestable, due to the action-at-a-distance problem):

public class FooFactory {
  private List myList;
  public FooFactory(List myList) {
    // ideally probably create an immutable copy of it
    this.myList = Collections.unmodifiableList(new ArrayList(myList));
    // ensure that myList has all the required characteristics
    if (this.myList.isEmpty()) {
      throw new IllegalArgumentException("Can't build a FooFactory with an empty myList");
    }
  }

  public Foo newFoo() {
    return new Foo(myList);
  }
}

This, together with a package-private Foo constructor (so that noone can go the direct, dangerous route) ensures that only proper Foo objects will be created.

• no action-at-a-distance, you always explicitly reference the FooFactory that contains the state
• no accidentally changing someone elses FooFactory (since it's immutable)
• you can't accidentally create a broken Foo object

Answer 5

Another possible solution would be to use the Singleton design pattern.

A real world example would be if a worker has a task to do and that task requires the use of a shared list why should the manager have to tell the worker what list to use when his/her task already knows which list is needed?

Make the myList class a singleton then in the constructor of the Foo class have it get a pointer to the one instance of the myList object.

Class myList
{
  static myList * get_instance()
  {
    static myList m_instance;
    return &m_instance;
  }
};

Class Foo
{
  public Foo()
  {
    myList * m_list = MyList::get_instance();
  }
};

This solution is the simplest for the requirements stated because it does not required the code creating the object to even care about the shared list but simply creates the Foo object.