Constructor versus setter injection

Question

Hi,

I'm currently designing an API where I wish to allow configuration via a variety of methods. One method is via an XML configuration schema and another method is through an API that I wish to play nicely with Spring.

My XML schema parsing code was previously hidden and therefore the only concern was for it to work but now I wish to build a public API and I'm quite concerned about best-practice.

It seems that many favor javabean type PoJo's with default zero parameter constructors and then setter injection. The problem I am trying to tackle is that some setter methods implementations are dependent on other setter methods being called before them in sequence.

I could write anal setters that will tolerate themselves being called in many orders but that will not solve the problem of a user forgetting to set the appropriate setter and therefore the bean being in an incomplete state.

The only solution I can think of is to forget about the objects being 'beans' and enforce the required parameters via constructor injection.

An example of this is in the default setting of the id of a component based on the id of the parent components.

My Interface

public interface IMyIdentityInterface {
    public String getId();
    /* A null value should create a unique meaningful default */
    public void setId(String id);
    public IMyIdentityInterface getParent();
    public void setParent(IMyIdentityInterface parent);
}

Base Implementation of interface:

public abstract class MyIdentityBaseClass implements IMyIdentityInterface {
   private String _id;
   private IMyIdentityInterface _parent;

   public MyIdentityBaseClass () {}

   @Override
   public String getId() {
      return _id;
   }

   /**
    * If the id is null, then use the id of the parent component
    * appended with a lower-cased simple name of the current impl
    * class along with a counter suffix to enforce uniqueness
    */
   @Override
   public void setId(String id) {
      if (id == null) {
          IMyIdentityInterface parent = getParent();
          if (parent == null) {
              // this may be the top level component or it may be that
              // the user called setId() before setParent(..)
          } else {
              _id = Helpers.makeIdFromParent(parent,getClass());
          }
      } else {
          _id = id;
      }
   }

   @Override
   public IMyIdentityInterface getParent() {
      return _parent;
   }

   @Override
   public void setParent(IMyIdentityInterface parent) {
      _parent = parent;
   }

}

Every component in the framework will have a parent except for the top level component. Using the setter type of injection, then the setters will have different behavior based on the order of the calling of the setters.

In this case, would you agree, that a constructor taking a reference to the parent is better and dropping the parent setter method from the interface entirely? Is it considered bad practice if I wish to be able to configure these components using an IoC container?

Chris

Answer 1

Its not a bad practice to use IOC container for this purpose. The choice of using a contrucxtor or setter injection is driven by the fact that whether any instnces of this classes should ever be created without the parent. Since in your case parent always needs to be set (except for the top node) constructor injection makes lot of sense

Answer 2

I don't really think the problem is setter vs. constructor injection. The problem is you're using null to mean too many things...

As for : /* A null value should create a unique meaningful default */

Using it as a "magic" value in the setId() method is also something that really seems like a bad code smell. If you never call setId() then getId() returns null, but if you call setId(null) then getId() returns some generated value?

Something like this would seem to make more sense:

public abstract class MyIdentityBaseClass implements IMyIdentityInterface {
    private String _id;
    private IMyIdentityInterface _parent;

    public MyIdentityBaseClass () {}

    @Override
    public String getId() {
        return _id;
    }

    @Override
    public void setId(String id) {
        _id = id;
    }

    @Override
    public IMyIdentityInterface getParent() {
        return _parent;
    }

    @Override
    public void setParent(IMyIdentityInterface parent) {
        _parent = parent;
        if (_id == null) {
            // if id isn't already set, set it to the generated id.
            _id = Helpers.makeIdFromParent(parent, getClass());
        }
    }

}

If your model really requires a parent feel free to use constructor injection, that's what it's there for. But realize you're still going to have to provide a default constructor for top level items... and then what's to stop someone from using the default constructor for an item that's not top level and then call setParent() on it later. I'd look at creating some kind of Container/Component architecture and make a special class (or classes) for the top level items. (You may have this already, as you've only given one example class and an abstract one at that.)

BTW - Also, the IInterface and _name conventions aren't Java naming conventions. If this is an exposed API that other people are supposed to use expect some complaining.

Answer 3

I could write anal setters that will tolerate themselves being called in many orders but that will not solve the problem of a user forgetting to set the appropriate setter and therefore the bean being in an incomplete state.

The Spring-specific solution to this is to declare the bean as implementing InitializingBean and then implement the necessary checking and back-filling of defaults in the bean's afterPropertiesSet() method. You can even wire this up so that programmers have the choice of using constructors with arguments or setters, and (maybe) have the argument-full constructors call afterPropertiesSet().

The only gotchas are:

• your codebase becomes Spring dependent by virtue of implementing InitializingBean, and

• if a developer configures the beans by hand using the setters they have to remember to call afterPropertiesSet() when they are done.

EDIT

The afterPropertiesSet() method also gives a neat solution to your example with calculating the id. The problem with the way you have currently written it is that it implicitly constrains you to building the tree from the top (root) down. For example, suppose you have a multi-level tree, and you set a leaf node's parent attribute before you set the parent node's parent attribute. This will trigger the id setting algorithm in the leaf node, which will wrongly think that the parent node is a root node.

In Spring you can arrange that the setting of the parent attributes is done in the implementation of the relevant setChild / setChildren methods of the parent nodes. The setting of the id attributes would then be done in the afterPropertiesSet() method. Since this gets called after all explicit and auto-wire property settings have been applied, the parent/child relationships for the tree will have stabilized, and so the id calculations will be correct.

Answer 4

It sounds like the problem is with the default ID. It would make your life much much easier if the bean doesn't decide its ID by itself. I would say the ID is the responsibility of the client code that constructs or uses the bean.

Of course, I don't know the exact context, but I assume the ID should be somewhat unique (in the bean's own context). Your current solution indicates that the ID is set in two different places: either in setId() method or in the client code.

In general case it is much better to have a single place that takes care of managing such IDs. Setting the ID to a default value inside of the bean might actually hide some other bugs.

If you really really think it is a problem that the future client code might not use your bean correctly, then I would rather validate the state of the bean in the getters or setters and throw a runtime exception like IllegalStateException or IllegalArgumentException.