23
votes
\$\begingroup\$

So I have a series of objects, which I will call Impl1, Impl2, and Impl3. They each implement an interface, called IImpl. I have a Factory class who's task is to retrieve the ImplX which is appropriate for a given circumstance, and pass it on to its callers. So the code in the factory looks like:

public IImpl GetInstance(params object[] args)
{
    if (args[0]=="Some Value")
         return new IImpl1();
    else if (args[0]=="Other Value")
         return new IImpl2(args[1]);
    else
         return new IImpl3(args[1]);
}

So depending on the arguments passed in, different instances are selected. All well and good and works ok. The problem now is, that I have a class which needs to call this factory method. It has no references to IImplX, which is good, but it winds up having to know exactly how to construct the input array to GetInstance, in order to ensure it receives the correct kind of instance. Code winds up looking like:

switch (_selectedInputEnum)
{
    case InputType.A:
        myIImplInst = Factory.GetInstance("Some Value");
    case InputType.B:
        myIImplInst = Factory.GetInstance("Other Value",this.CollectionB);
    case InputType.C:
        myIImplInst = Factory.GetInstance("Third Value",this.CollectionC);
}

This feels very redundant, and off somehow. What would be the best way to abstract the actual parameters of the factory? I feel like with the above switch statement, I am strongly coupled to the implemenations of IImplx, even if I don't have a direct reference.

\$\endgroup\$

locked by Jamal Apr 22 '16 at 3:05

This question exists because it has historical significance, but it is not considered a good, on-topic question for this site so please do not use it as evidence that you can ask similar questions here. This question and its answers are frozen and cannot be changed. See the help center for guidance on writing a good question.

Read more about locked posts here.

  • \$\begingroup\$ I think either we need more information, or you're expectations can't be met: if you are required to pass different kinds of parameters to this factory for it to work properly, then the client code always has to provide these parameters, and therefore always has to know which overload to call or which parameters to supply. \$\endgroup\$ – Alex ten Brink Jan 22 '11 at 13:52
9
votes
\$\begingroup\$

How about some kind of intermediary blackboard that the client code constructs before calling the factory, and each concrete Impl can poll to construct itself?

// client code:

Blackboard blackboard = new Blackboard();
blackboard.pushCollectionB(collectionB);
blackboard.pushCollectionC(collectionC);
blackboard.pushFoo(foo); 

IImpl myInstance = Factory.GetInstance(b);

///////////////////////////

// in Factory.GetInstance():

return new Impl3(blackboard);

////////////////////////////

// in Impl3:

Impl3(Blackboard b) { process(b.getCollectionC()); }

I've hidden the switch statement in the client code, but you could move that into the blackboard as well.

What data each concrete Impl needs is now hidden from both the Factory and the client code. However if you need more data in your Blackboard for Impl(x+1) you will need to update every place in your code that creates a Blackboard.

Depending on application, over-constructing the Blackboard like this may be expensive for you. You could construct a cached version at startup, or you could make the Blackboard an interface and have your client code derive from it.

\$\endgroup\$
  • \$\begingroup\$ In this construct, the factory itself would have to know which ImplX to call, presumably by polling the blackboard itself? \$\endgroup\$ – GWLlosa Jan 21 '11 at 14:45
  • \$\begingroup\$ You could go either way, leaving the switch in the client code to translate from InputType, and passing the string into the Factory alongside the Blackboard, or push it into the Blackboard as you suggest. Either way works. \$\endgroup\$ – tenpn Jan 21 '11 at 15:17
7
votes
\$\begingroup\$

Where are CollectionB and CollectionC coming from? The approach I'd shoot for is to have a variety of factories that all accept the same type of input, and then store them in some type of collection. For example, if one has a list of objects to be created, one object per line, and the portion of each line before the first blank defines the type of each item, one could have a factories that take a string and yield an appropriately-configured object. For example, if the input looked like:

square 0,5,5,29
line 5,2 19,23 6,8
text 0,29,Hello there!

one could have a SquareFactory, LineFactory, and TextFactory all of which inherit from GraphicObjectFactory, which accept a string and return GraphicObject. One could then have a Dictionary that maps String to GraphicsObjectFactory, and put in it an instance of each of the above mentioned factories. To allow the file reader to handle more types of graphics objects, just add more factories into the Dictionary.

\$\endgroup\$
  • \$\begingroup\$ Unless I'm misreading this, it seems like a similar concept to tenpn's answer, which is near to what I did, so I like it :) \$\endgroup\$ – GWLlosa Jan 29 '11 at 2:03
  • \$\begingroup\$ It's somewhat like tenpn's answer, except that his "blackboard" is configured by code in various clients of the class, while I would expect to have the factory constructed from a configuration file or other single point of definition. \$\endgroup\$ – supercat Jan 30 '11 at 23:40
1
vote
\$\begingroup\$

It seems to me that the factory should be choosing the ImplX based on arg[1] then instead of arg[0].

So something like this would remove the need for the switch, arg[1] is now arg[0] in this example:

public IImpl GetInstance(params object[] args)
{
    if (args.length == 0)
         return new IImpl1();
    else if (args[0] is IEnumerable<string>)
         return new IImpl2(args[0]);
    else
         return new IImpl3(args[0]);
}

You could then call it like:

var impl1 = GetInstance();
var impl2 = GetInstance(new List<string>{"1","2"});
var impl3 = GetInstance("");

Edit: If you don't want the caller to have to know the inner works of it then you should expose overloads for getInstance so:

public IImpl GetInstance()
{ 
    return new Impl1();
}
public IImpl GetInstance(IEnumberable<string> strings)
{
    return new Impl2(strings);
}
public IImpl GetInstance(string string)
{
    return new Impl3(string);
}
\$\endgroup\$
  • 1
    \$\begingroup\$ That still requires that the caller of GetInstance (your second code block in this case) know about the internals of how GetInstance works (in this case, knowing to pass different parameters in different orders to GetInstance). \$\endgroup\$ – GWLlosa Jan 21 '11 at 19:52
  • \$\begingroup\$ Then I think you will need to create some overrides for GetInstance that support the different calling methods. \$\endgroup\$ – Sean Lynch Jan 21 '11 at 20:02
  • 1
    \$\begingroup\$ Wouldn't they still have to know which override to call? \$\endgroup\$ – GWLlosa Jan 21 '11 at 20:31
  • \$\begingroup\$ Are they supposed to be able to decide what ImplX they are getting? If so then instead of GetInstance(IEnumerable<string> strings) it would be GetInstanceImpl2(IEnumerable<string> strings). \$\endgroup\$ – Sean Lynch Jan 21 '11 at 20:37
1
vote
\$\begingroup\$

Seems like your abusing the factory pattern, the whole purpose of a factory is that you pass it some general object and the factory decides what is most appropriate to return. The passed object doesnt need to know about the factory but the factory should know about the object it's passed.

If you need to pass it very explicit parameters then that is a code smell in your architecture and I'd think seriously about doing some refactoring rather than just "fixing" this problem

\$\endgroup\$
  • 1
    \$\begingroup\$ That's the exact intent of this question; refactoring this into a better solution. \$\endgroup\$ – GWLlosa Jan 23 '11 at 14:43
0
votes
\$\begingroup\$

It says this question was first posted 5 years ago, but no one ever answered it so here goes.

First of all, let's get a bit more detail into the example. I suggest we use an IPersistable interface which supports one public method named Save(). It'll look like the following:

interface IPersistable {
    bool Save();
}

Next, we want three classes which implement this method:

class FileSaver : IPersistable{
    public bool Save(){
        Console.WriteLine("I'm saving into a FILE.");
        return true;
    }
}

class DatabaseSaver : IPersistable{
    public bool Save(){
      Console.WriteLine("I'm saving into a DATABASE.");
      return true;
    }
}

class TcpSaver : IPersistable{
    public bool Save(){
        Console.WriteLine("I'm saving into a WEB LOCATION.");
        return true;
    }
}

Finally, we need a factory class which will build our appropriate class on demand.

class SaverFactory{
    private string type;
    public SaverFactory(string type){
        this.type = type;
    }

    public IPersistable CreateSaver(){
        switch (type){
            case "FileSaver" :
            {
                return new FileSaver();
            }
            case "DatabaseSaver" :
            {
                return new DatabaseSaver();
            }
            case "TcpSaver" :
            {
                return new TcpSaver();
            }
            default : 
                return null;
        }
    }
}

You can now copy that C# code and paste it into a LINQPad (http://linqpad.net) session and add the following main method and it will work beautifully.

void Main()
{
    List<IPersistable> allItems = new List<IPersistable>();
    List<String> fakeData = new List<String>();
    fakeData.Add("FileSaver"); fakeData.Add("DatabaseSaver");

    foreach (String s in fakeData){
        IPersistable ip = new SaverFactory(s).CreateSaver();
        if (ip != null)
            allItems.Add(ip);
    }

    foreach (IPersistable ip in allItems)
    {
        ip.Save();
    }
}

Code Run Result

If you run that code you will see the following:

I'm saving into a FILE.

I'm saving into a DATABASE.

That is a proper working Factory method. It is dependent upon the type string that you pass in. That is expected, because you are telling it that you are building a specific type and that is expected.

What If Dependent Type Needs Parameters For Configuration?

However, what you are wondering now is what happens if the type you are requesting to be built by the factory needs some other parameters so that it can configure itself.

For example in this case I would want to send in a database connection for the DatabaseSaver and a file path and name for the FileSaver and a URI for the TcpSaver. You believed that providing that configuration information for building the implementation object somehow sullied your design, but I don't believe it does.

I mean the whole point of a factory is that something tells it to build a specific implementation class. In my case I create a list of strings which are then passed in to the factory. Imagine those strings were loaded with other information which configures my IPersistable implementation. That would be fine.

Let's move the example ahead a bit by showing how I might pass the needed configuration item into the factory so the IPersistable implementation class can be constructed around that extra configuration information.

Create New Interface and Classes : IConfigurable

interface IConfigurable{
    String type{get;set;}
}

class FileConfig : IConfigurable{
    public String type{get;set;}
    public string FileName{get;set;}
    public FileConfig(String type, String fileName=null){
        this.type = type;
        FileName = fileName;
    }

}
class DatabaseConfig : IConfigurable{
    public String type{get;set;}
    public string ConnectionString{get;set;}
    public DatabaseConfig(String type, String ConnectionString=null){
        this.type = type;
        this.ConnectionString = ConnectionString;
    }
}

class TcpConfig : IConfigurable{
    public String type{get;set;}
    public string Uri {get;set;}
}

I became a little lazy on that last one and didn't implement it all because I know no one will read all of this. I will leave you with the entire code listing which you can run and examine using LINQPad.

void Main()
{
    List<IPersistable> allItems = new List<IPersistable>();
    List<IConfigurable> fakeData = new List<IConfigurable>();
    fakeData.Add(new FileConfig("FileSaver",@"c:\superpath")); 
    fakeData.Add(new DatabaseConfig("DatabaseSaver",@"connection=superdb;integrated security=true"));

    foreach (IConfigurable ic in fakeData){
        IPersistable ip = new SaverFactory(ic).CreateSaver();
        if (ip != null)
            allItems.Add(ip);
    }

    foreach (IPersistable ip in allItems)
    {
        ip.Save();
    }
}

interface IPersistable {
    bool Save();
}

interface IConfigurable{
    String type{get;set;}
}

class FileConfig : IConfigurable{
    public String type{get;set;}
    public string FileName{get;set;}
    public FileConfig(String type, String fileName=null){
        this.type = type;
        FileName = fileName;
    }

}
class DatabaseConfig : IConfigurable{
    public String type{get;set;}
    public string ConnectionString{get;set;}
    public DatabaseConfig(String type, String ConnectionString=null){
        this.type = type;
        this.ConnectionString = ConnectionString;
    }
}

class TcpConfig : IConfigurable{
    public String type{get;set;}
    public string Uri {get;set;}
}

class FileSaver : IPersistable{
    IConfigurable config;
    public FileSaver(IConfigurable config){
        this.config = config;
    }
    public bool Save(){
        Console.WriteLine("I'm saving into a FILE.");
        var configItem = config as FileConfig;
        Console.WriteLine(configItem.FileName);
        return true;
    }
}

class DatabaseSaver : IPersistable{
    IConfigurable config;
    public DatabaseSaver(IConfigurable config){
        this.config = config;
    }
    public bool Save(){
        Console.WriteLine("I'm saving into a DATABASE.");
        var configItem = config as DatabaseConfig;
        Console.WriteLine(configItem.ConnectionString);
        return true;
    }
}

class TcpSaver : IPersistable{
    IConfigurable config;
    public TcpSaver(IConfigurable config){
        this.config = config;
    }
    public bool Save(){
        Console.WriteLine("I'm saving into a WEB LOCATION.");
        return true;
    }
}

class SaverFactory{
    IConfigurable config;
    public SaverFactory(IConfigurable config){
        this.config = config;
    }

    public IPersistable CreateSaver(){
        switch (config.type){
            case "FileSaver" :
            {
                return new FileSaver(config);
            }
            case "DatabaseSaver" :
            {
                return new DatabaseSaver(config);
            }
            case "TcpSaver" :
            {
                return new TcpSaver(config);
            }
            default : 
                return null;
        }
    }
}
\$\endgroup\$
  • 2
    \$\begingroup\$ Errr, did you post to the right question? The code you are reviewing has nothing to do with the code in the question. Also, there are 4 answers already so it was answered. \$\endgroup\$ – Tunaki Apr 21 '16 at 20:45
  • \$\begingroup\$ I know. I know it's 5 years old. I thought the OP wanted an explanation of the Factory pattern and how it is possible to configure. OP's original code made it very difficult for others to answer and needed some details behind it. My sample runs and shows that you can send in params to the Factory to create your implementation classes and that sending in params can be a valid part of using the Factory pattern. \$\endgroup\$ – raddevus Apr 21 '16 at 20:49
  • \$\begingroup\$ This old question was off-topic at the time (and with good answers) and is no longer off-topic, thus it's been historical locked. If you would like to discuss it with the OP more (if he/she sees this), then it can be taken to chat. \$\endgroup\$ – Jamal Apr 22 '16 at 3:08

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