4

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);
}
  • 3
    Any change for you to change the superclass? For instance, implementing a single constructor with Optional<Long> fileSize instead of long fileSize? Or what Doc said. add a private method to setUp() the common part. – Laiv Apr 12 '18 at 11:46
  • @KaranKhanna are these attributes final? – Laiv Apr 12 '18 at 11:51
  • If you call super with no filesize, what does it use for it? Maybe your one-arg constructor could just call the two-arg passing (transaction, 0) (or -1, whatever, make it a constant named DEFAULT_FILESIZE) – user949300 Apr 12 '18 at 16:50
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.

7

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.

  • 1
    This won't work as the fields are final and thus can be initialized only in the constructor. – Karan Khanna Apr 12 '18 at 16:03
  • Not allowed with Java. You will get "cannot assign a value to final variable". – Karan Khanna Apr 12 '18 at 16:11
  • Yeah, I think you're right about that. You could try passing your final values in as parameters to the new function, but at that point, you might as well just leave the code the way it is. The other thing you could do is simply make those variables not final. Is there any real danger of those private variables being replaced by new objects? – Robert Harvey Apr 12 '18 at 16:17
  • Probably I would refactor my super class. Not a great design as I can see it. :-( – Karan Khanna Apr 12 '18 at 16:19
  • 2
    Then I would simply live with the code duplication. Java is not a concise language anyway, and all you're doing is making assignments. – Robert Harvey Apr 12 '18 at 16:19

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.