Static factory method gateway to internal factory - code smell?

Question

Say I had a class that has a static factory method, like this:

 public class Table
 {
    public static Table OpenTable(string path)
    {
        ITableFactory fac = IoC.Resolve<ITableFactory>();
        return fac.OpenTable(path);
    }
 }

and a factory class that looks like this:

internal class TableFactory : ITableFactory
{
    internal Table OpenTable(string path)
    {
      //Check the path
      //Do some other stuff
      //return a new Table.
    }
}

Does this code smell bad to you?

EDIT: Another question: Is it a good idea to have a static method on the type that just forwards calls to the factory?

Some background: I used to have the TableFactory as public and make the user create a new one every time they needed to open a table but it felt like a long hall just to open a table. So I thought that I would make a static factory method on the Table class and make the factory class internal and just resolve it using IoC.

Answer 1

That looks fine. I would however move fac out:

 public class Table
 {
    private static readonly ITableFactory fac = IoC.Resolve<ITableFactory>();
    public static Table OpenTable(string path)
    {
        return fac.OpenTable(path);
    }
 }

Answer 2

I don't think this is too bad... it's just a shortcut method that introduces no new dependencies.

I know in ninject you could possibly do Kernel.Get<Table>(path) and avoid all this juggling.

Answer 3

I don't think it's bad, per se, but perhaps a little unusual.

I would, however, present the table factory as a separate object to the user, and use that to create tables. Why ? It's a little more conventional, and it allows you the capability to be able to configure your factory in advance and use it for creating more than one table e.g.

TableFactory tf = new TableFactory();
tf.setWhatever();
tf.setWhatever2();
// ...etc...

// now create your tables based on the above.

Your code need not (necessarily) know anything about the table creation, other than the fact that it's been given a TableFactory (built and configured elsewhere)

That may be overkill for what you're doing. But I've found it's a useful pattern to follow.

Answer 4

As far as I can tell, what you have here is an instance of the Service Locator pattern, since I assume that IoC is a type name, and Resolve is a static method on the IoC type. Whether your TableFactory is internal or not is of less concern.

I consider Service Locator to be an anti-pattern, since it is totally opaque to the user of the API which dependencies need to be in place; thus, one could easily invoke Table.OpenTable in a context where the call to IoC.Resolve would throw, and the API gives you absolutely no clue that this is the case. You also get no compile-time checking.

I know Martin Fowler originally described the Service Locator pattern, so it's a pretty harsh thing to call it an anti-pattern, but I know I'm not alone in this. YMMV.