Where should I put the common code of the constructors?

Question

I have a situation where in a class I have 2 constructors and they have very similar code. The only difference is the the call to the constructor of the super class. Where should I put this common code? I tried to use the instance block but with instance blocks I am not able to pass arguments required.

Also, the fields are final. So wont be able to initialize other then the constructor.

My code looks like:

private final SourceCache sourceCache;
private final ServiceCache serviceCache;
private final MethodCache methodCache;
private final ModelCache modelCache;
private final QueryFactory queryFactory;

public MetaDataPersistenceHandler(
    final Transaction transaction)
{
    super(transaction);
    this.transaction = transaction;
    this.sourceCache = new SourceCache(transaction);
    this.serviceCache = new ServiceCache(transaction);
    this.methodCache = new MethodCache(transaction);
    this.modelCache = new ModelCache(transaction);
    this.queryFactory = new QueryFactory();
    this.transaction.addQueryFactory(this.queryFactory);
}



public MetaDataPersistenceHandler(
    final Transaction transaction,
    final long fileSize)
{
    super(transaction, fileSize);
    this.transaction = transaction;
    this.sourceCache = new SourceCache(transaction);
    this.serviceCache = new ServiceCache(transaction);
    this.methodCache = new MethodCache(transaction);
    this.modelCache = new ModelCache(transaction);
    this.queryFactory = new QueryFactory();
    this.transaction.addQueryFactory(this.queryFactory);
}

Answer 1

In the superclass, there is (very likely) a default value for the fileSize parameter. We can take advantage of that fact here.

First, we need to get an idea of what that default value might be. We could see something like:

public class Handler {
    public static final long DEFAULT_FILE_SIZE = 4096;
    private final long fileSize;

    public Handler(Transaction tr) {
        this(tr, DEFAULT_FILE_SIZE)
    }

    public Handler(Transaction tr, long fsize) {
        this.fileSize = fsize;
        // ...
    }
}

This makes is particularly easy, especially if that constant is visible to the subclass:

public MetaDataPersistenceHandler(Transaction tr) {
    this(tr, Handler.DEFAULT_FILE_SIZE);
}

public MetaDataPersistenceHandler(Transaction tr, long fileSize) {
    super(tr, fileSize);
    // etc...
}

Alternatively, if there is not a constant, but there is a simple value, you could extract it to a constant in the superclass and then reference it.

If the logic around the default value is a little more complicated, you could instead extract out a visible static method in the superclass:

protected static long defaultFileSize(Transaction tr) {
    if (tr.isHuge()) {
        return 4096;
    } else {
        return 128;
    }
}

And call it from your subclass constructor in a similar manner:

public MetaDataPersistenceHandler(Transaction tr) {
    this(tr, Handler.defaultFileSize(tr));
}

public MetaDataPersistenceHandler(Transaction tr, long fileSize) {
    super(tr, fileSize);
    // etc...
}

These changes to the two classes have the benefit of preserving the public interface exposed. None of the signatures of any of the constructors have to change. And we can keep our class members final like we want.

If you cannot modify the superclass, but the default value is unlikely to change, you could always put the default value into the subclass as a constant or static method, with loud comments indicating the duplication with the superclass.

---

That's nice and all, but the superclass actually does two completely different things between the superconstructors. Ouch, that's not really good design, but OK, we can still do something. This also applies if you don't want the duplication because you cannot modify the superclass.

Instead of inheritance, we could do some composition.

This works best if there is a super-interface above your superclass for what you really want, but there are ways to do it with just a superclass if it is not too hostile. Say we have a Handler interface and an SimpleHandler superclass. Our class can implement Handler instead of extending SimpleHandler, and we can change our constructor to take a Handler instance:

public class MetaDataPersistenceHandler implements Handler {
    private final Handler handler;
    // other memebers...

    public MetaDataPersistenceHandler(Handler handler) {
        this.handler = handler;
        // etc...
    }
}

Instead of our original constructor calls, we would do:

new MetaDataPersistenceHandler(new SimpleHandler(transaction));
new MetaDataPersistenceHandler(new SimpleHandler(transaction, fileSize));

The problem now is that we have to implement all the methods of Handler ourselves. To do this, we create all of the methods for the Handler interface, and delegate them to our member Handler instance:

@Override
public void handle() {
    this.handler.handle();
}

This can look a little boilerplate-ish, unfortunately, but it is the simplest way to do the delegation. For that, though, we don't need to worry about modifying the superclass, or if some way it handles the default file size changes. Our external interface of the subclass changed since we had to change the constructor signature, so beware of that.

If you want to go this route but really can't change those constructor signatures, there's even a way around that:

public class MetaDataPersistenceHandler implements Handler {
    private final Handler handler;
    // other memebers...

    private MetaDataPersistenceHandler(Handler handler) {
        this.handler = handler;
        // etc...
    }

    public MetaDataPersistenceHandler(Transaction tr) {
        this(new SimpleHandler(tr));
    }

    public MetaDataPersistenceHandler(Transaction tr, long fileSize) {
        this(new SimpleHandler(tr, fileSize));
    }

    // etc...
}

---

Maybe I'm just not getting this right, though. Handler is an abstract "strategy"-type class. It has a few abstract methods for us to override, and a bunch of final methods we can't. I think we can still work with this, we just need to flip the composition relationship.

Let's first make an interface for all the abstract methods in Handler:

public interface BasicHandler {
    void handle();
}

Now lets create a simple handler that will implement the abstract methods and delegate them to a BasicHandler it takes via its constructor:

public class DelegatingHandler extends Handler {
    private final BasicHandler handler;

    public DelegatingHandler(Transaction tr, BasicHandler handler) {
        super(tr);
        this.handler = handler;
    }


    public DelegatingHandler(Transaction tr, long fileSize, BasicHandler handler) {
        super(tr, fileSize);
        this.handler = handler;
    }

    @Override
    public void handle() {
        this.handler.handle();
    }
}

And we can write our own implementation of BasicHandler:

public class MetaDataPersistenceBasicHandler implements BasicHandler {
    // members...

    public MetaDataPersistenceBasicHandler(Transaction tr) {
        // Everything that goes here...
    }

    @Override
    public void handle() {
        // our special way to handle...
    }
}

Now we can create Handlers like:

new DelegatingHandler(tr, new MetaDataPersistenceBasicHandler(tr));
new DelegatingHandler(tr, fileSize, new MetaDataPersistenceBasicHandler(tr));

Once again, we've changed the constructor signature here.

Answer 2

You put the duplicate code in a new private method, just like you would in any other refactoring of duplicate code.

private SourceCache sourceCache;
private ServiceCache serviceCache;
private MethodCache methodCache;
private ModelCache modelCache;
private QueryFactory queryFactory;

public MetaDataPersistenceHandler(final Transaction transaction)
{
    super(transaction);
    AddCachesAndFactoriesTo(transaction);
}

public MetaDataPersistenceHandler(final Transaction transaction, final long fileSize)
{
    super(transaction, fileSize);
    AddCachesAndFactoriesTo(transaction);
}

private AddCachesAndFactoriesTo(Transaction transaction)
{
    this.transaction = transaction;
    this.sourceCache = new SourceCache(transaction);
    this.serviceCache = new ServiceCache(transaction);
    this.methodCache = new MethodCache(transaction);
    this.modelCache = new ModelCache(transaction);
    this.queryFactory = new QueryFactory();
    this.transaction.addQueryFactory(this.queryFactory);
}

You'll have to give up your final modifiers on your private variables, though.