Java Simple Factory with constructors using different parameters

Question

I have two ways of saving data in my application: save to database and save to file. Since I don't want client code dealing with construction of objects I created a class that (to my understanding) is simple factory with a factory method. Code below:

public static DataPersister createDataPersister(Boolean saveToDb, Session session, String filename) {
    if (saveToDb) {
        return new DatabaseDataPersister(session);
    } else {
        return new FileDataPersister(filename);
    }
}

With this setup client code doesn't have to deal with constructing anything or deciding whether to save to DB or file - it can just call a save() method on an object returned by the factory like so:

DataPersister dataPersister = DataPersisterSimpleFactory.createDataPersister(this.savetoDb, this.session, this.filename);
dataPersister.save(this.data);

My question is - is this solution breaking SOLID principles? In order to create e.g. a DatabaseDataPersister client code needs to pass on a filename parameter, and this implementation of DataPersister won't have any use of it. I feel like it doesn't sit right with something similar to Interface-segregation principle but not quite that.

And if the solution is indeed a code smell - how do I go about cleaning it?

weston · Accepted Answer · 2018-04-21 13:46:31Z

3

The SOLID principle I think is in violation is DIP.

Your client classes, by having to depend on the static factory directly, have a compile-time dependency on actual implementations, DatabaseDataPersister and FileDataPersister, rather than just the abstraction DataPersister.

To solve, supply to the client the DataPersister you want them to use. The constructor is usually a good place for this:

public class ExampleClient {

    private final DataPersister dataPersister;

    public ExampleClient(DataPersister dataPersister) {
        this.dataPersister = dataPersister;
    }

    public void methodThatUsesSave(){
        dataPersister.save(data);
    }
}

This code compiles without the concrete implementations, i.e. it has no dependency on them. The client also doesn't need to know the filename or session so it solves that code smell too.

We can decide which concrete implementation to give it at construction time, here I use your existing method:

DataPersister dataPersister = DataPersisterSimpleFactory.createDataPersister(this.savetoDb, this.session, this.filename);
ExampleClient example = new ExampleClient(dataPersister);

answered Apr 21, 2018 at 13:46

weston

55k23 gold badges151 silver badges209 bronze badges

Sign up to request clarification or add additional context in comments.

1 Comment

naru Over a year ago

I find your comment the most helpful. I can easily inject the correct DataPresister based on a command line argument. Thanks!

OldCurmudgeon · Accepted Answer · 2018-04-20 14:58:00Z

2

This is a perfect opportunity to use the factory pattern

interface DataPersister {
    void persist(String s);
}

private class DatabasePersister implements DataPersister {
    final Session session;

    public DatabasePersister(Session session) {
        this.session = session;
    }

    @Override
    public void persist(String s) {
        System.out.println("Persist to database: " + s);
    }
}

private class FilePersister implements DataPersister {
    final String filename;

    public FilePersister(String filename) {
        this.filename = filename;
    }

    @Override
    public void persist(String s) {
        System.out.println("Persist to file: " + s);
    }
}

class PersisterFactory {
    public DataPersister createDatabasePersister(Session session) {
        return new DatabasePersister(session);
    }

    public DataPersister createFilePersister(String filename) {
        return new FilePersister(filename);
    }
}

public void test(String[] args) {
    DataPersister databasePersister = new PersisterFactory().createDatabasePersister(new Session());
    databasePersister.persist("Hello");
    DataPersister filePersister = new PersisterFactory().createFilePersister("Hello");
    filePersister.persist("Hello");
}

answered Apr 20, 2018 at 14:58

OldCurmudgeon

66k18 gold badges126 silver badges220 bronze badges

1 Comment

naru Over a year ago

Your code basically looks like mine except you handle the decision which concrete implementation of DataPersister to create in the client code (in your example, the test() method) and I wish to push the decision making somewhere else (in my example the factory decides what to create based on saveToDb boolean - which is by the way set based on a command line argument so the client doesn't decide on that either). Can you think of a solution where client code doesn't have to devcide? Is it possible, or even justifiable?

Dave Newton · Accepted Answer · 2018-04-20 14:23:02Z

1

You already pass a bunch of stuff irrelevant to various persisters.

As it stands you need a method that takes a Session and one that takes a String and you're done. No need for a boolean, no need for useless params. That handles your decision making with no cruft.

Whether or not that's a good idea... I'm ambivalent. You're not saving much; might as well just have a static factory in each type so it's explicit in the code what type you're creating.

Consider what happens when you add a new persister, like a REST endpoint, that would take a URL (could be a string, could be an actual URL). You now need even more useless parameters etc. Or you could pass in a URI from the beginning, e.g., file:// or http:// and get around that problem.

There are any number of ways this could be done–I'm not convinced there's a "clearly correct" answer, and it may boil down to opinion.

answered Apr 20, 2018 at 14:23

Dave Newton

161k27 gold badges263 silver badges310 bronze badges

2 Comments

naru Over a year ago

The boolean variable saveToDb is used by the factory to decide on which concrete implementation to create. Writing two separate methods for creation of two seperate implementations, like in @OldCurmudgeon answer, while helpful with getting rid of one variable, forfeits my goal of the client code not needing to decide which DataPersister implementation to instantiate.

Dave Newton Over a year ago

@naru You're trading methods for variables-there's no logical difference; the client still decides. Possible? Sure-always pass all the variables: the DB doesn't need the filename, the file doesn't need the session. The other answer just needs a function that decides based on the parameter type, which is what I said as well.

inf3rno · Accepted Answer · 2020-01-14 05:12:01Z

Well the right solution here is combining the dependency injection from weston and the factory pattern from OldCurmudgeon.

public class ExampleClient {

    private final DataPersister dataPersister;

    public ExampleClient(DataPersister dataPersister) {
        this.dataPersister = dataPersister;
    }

    public void methodThatUsesSave(){
        dataPersister.save(data);
    }
}

class PersisterFactory {
    public DataPersister createDatabasePersister(Session session) {
        return new DatabasePersister(session);
    }

    public DataPersister createFilePersister(String filename) {
        return new FilePersister(filename);
    }
}

The upper level code:

PersisterFactory = new PersisterFactory();
DataPersister dataPersister;
if (saveToDb)
    dataPersister = PersisterFactory.createDatabasePersister(new Session());
else
    dataPersister = PersisterFactory.createFilePersister("Hello");
ExampleClient example = new ExampleClient(dataPersister);

Usually the dataPersister comes from the DI container and the saveToDb comes from the config, but of course testing can be an exception.

Collectives™ on Stack Overflow

Java Simple Factory with constructors using different parameters

4 Answers 4

1 Comment

1 Comment

2 Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

4 Answers 4

1 Comment

1 Comment

2 Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related