4

I'm implementing a factory like this:

public interface IMyProduct
{
    void DoSomething();
}

public interface IMyFactory
{
    IMyProduct CreateMyProduct( string aParameter );
}

internal MyFactory : IMyFactory
{
    public MyFactory( ISomeService someService )
    {
        _someService = someService;
    }

    public IMyProduct CreateMyProduct( string aParameter )
    {
        return new MyProduct( _someService, aParameter );
    }

    private readonly ISomeService _someService;
    private class MyProduct : IMyProduct
    {
        public MyProduct( ISomeService someService, string aParameter )
        {
            // use the service and the parameter for something...
        }

        void IMyProduct.DoSomething()
        {
            // whatever...
        }
    }
}

That is, I put the product's implementation inside the factory's implementation.

The idea behind this is that MyProduct is referred to only by the IMyProduct interface (because the factory's interface exposes only the product's interface), so it doesn't need to be internal or even public.

Sure, the factory gets more complicated, but without the nested product class, it's basically trivial. My factory is like a complicated constructor implementation for the product, everything interesting is in the product (even the file might be named MyProduct.cs).

Now the question: what's the catch? I've never seen an abstract factory implemented this way, and I'm wondering what problem or risk I'm missing out on here.

Edit:

I pose the question differently: why are the products of factories seemingly never implemented as private classes? and what's the benefit of making the implementation of the product of a factory accessible to anyone besides the factory itself?

migrated from codereview.stackexchange.com Mar 6 '16 at 15:16

This question came from our site for peer programmer code reviews.

2

This is a nice and standard pattern if you want to control creation of IMyProduct.

I cannot see any issues in the code apart from weird formatting around parenthesis :).

A couple of things to think about:

  • The interface is public so anyone can implement it and create a type that does not obey the creation rules:
public class EvilProduct : IMyProduct
{
    ... not doing the creation dance.
}
  • An (abstract) class with ctor internal Foo(ISomeService someService, string aParameter) is an alternative.
  • Make sure you are solving a real problem by adding this so it is not just pattern-wanking and overengineering :).
  • I agree with you, but I think you miss the point - my product's implementation is a private nested class of the factory's implementation, not an internal or public class sitting next to the factory. – Haukinger Mar 5 '16 at 15:42
  • @Haukinger edited, still possible I'm missing the point of course. – Johan Larsson Mar 5 '16 at 15:59
2

I pose an opposing question : Why should it be a nested class?

The only real advantage of having a nested class is to allow that class access to it's parent's private properties. This allows slightly tighter access control than other options.

In your case, I don't see MyProduct calling the factory's privates. As such, I don't see the point of keeping it inside the factory class. If you don't want others to create the instances of MyProduct, then making it internal should provide big enough protection (assuming your projects are structured properly).

The reason why you don't often see it is because products generally don't need access to their factory's privates.

On the other side, I see nothing wrong keeping it like you have right now.

  • Good point, no need to access the factory from the product. I made the product private because noone needs to access it, and to reduce surface area of the assembly (so many views, viewmodels, even test fixtures are public without any reason)... – Haukinger Mar 7 '16 at 10:37
1

The point of a Factory class is to decouple a class's construction from its implementation. This allows us to create different implementations without knowing much about their concrete implementations.

What you have here is similar, but not identical. Your Factory is tied to a specific concrete implementation, and mainly hides its instantiation logic being an opaque facade. You get the more flexible interface of a factory rather than exposing a constructor (ctors can can either create a new instance or throw, not return a cached instance or null), but without the replacability of real factories.

It seems to me, then, that what you want is a Factory that's tied to a specific Type - and luckily, we have exactly such a pattern, which is the Factory Method. Instead of having a Factory object, simply have a private constructor and a static CreateProduct method that hides your construction logic and returns a new instance - or a cached instance, or null.

public class MyProduct : IMyProduct
{  
    public static MyProduct CreateInstance (string aParameter)
    {
        return new MyProduct(...);
    }

    private MyProduct( ISomeService someService, string aParameter )
    {
    }           
}

Back to the Question

As to your essential question - what's the catch? Complexity. If all you want is to hide construction details, just hide construction details! Adding a second class implementing a second interface just to create your one, specific, concrete class is more complicated, requires more code to implement (more code == more bugs!), and is more complicated for the user of your library (even if it's you - you in 3 months == a different user).

  • Of course, the factory produces one type of product, that's the whole point of having an abstract factory. I inject a concrete factory into an object to configure the type of objects that object creates during its lifetime (without knowing having to know the concrete type of the product, just the factory's interface and the product's interface). There are multiple variants of MyProduct (and multiple factory implementations)... – Haukinger Mar 6 '16 at 16:27

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.