CODEWITHSUNDEEP
c#design-patterns
user2079828
user2079828

Reputation: 655

Does holding information in a Factory instead of passing it through calling code make sense?

I have a factory which creates objects that are widely used through my application. Some of the objects the factory creates require another dependency. This optional dependency is an interface/class with settings from a local settings file. Currently both the factory interface AND the settings interface are injected into something like 20 classes. Albeit, the number of classes would be less if the applications inheritance tree wasn't about 6-7 levels deep (meaning the IFactory and ISettings get passed through 6 constructors and finally arrive at a base class which assigns them to a field).

The deep inheritance isn't getting changed anytime soon, but I thought that I could lessen the load of these constructors by removing the ISettings from them and putting it in the IFactory implementation itself. The ISettings would be injected into the concrete factory through the constructor, and then instead of passing the ISettings as a part of the Create() method, it would just use the factory ISettings readonly property. These settings are guaranteed to be immutable once the program starts.

Is this frowned upon in any way, and are there better ways of going about this? Some coworkers think that Factories should almost never hold state...but they aren't exactly sure why.

These are similar questions that I found, that gave answers but I'm still looking for a more fleshed out answer if possible.

Adding Properties to Abstract Factory

Abstract Factory Pattern and Properties

Simple example:

interface IObjValidator
{
    bool Validate();
}

interface IObjValidatorFactory
{
    IObjValidator Create();
}

class ObjValidatorFactory : IObjValidatorFactory
{
    // Is it OK for the factory to hold this field?
    private readonly ISettings settings;    

    public IObjValidator(ISettings settings)
    {
        this.settings = settings;
    }    

    IObjValidator Create(ObjValidatorMode mode)
    {
        switch(mode)
        {
             case ObjValidatorMode.One:
                 return ObjValidator1();

             case ObjValidatorMode.Two:
                 return ObjValidator2(this.settings);
        }
    }
}

class ObjValidator1: IObjValidator
{
    public bool Validate()
    {
        // Do stuff.
    }
}

class ObjValidator2: IObjValidator
{
    private readonly ISettings settings;

    public ObjValidator2(ISettings settings)
    {
        this.settings = settings;
    }

    public bool Validate()
    {
        // Use ISettings field data here.            
    }
}

Upvotes: 1

Views: 110

Answers (3)

Chetan Kinger
Chetan Kinger

Reputation: 15212

Is this frowned upon in any way, and are there better ways of going about this?

An incorrect approach to solving this problem would be to have a Singlteon settings class that the validators access using Settings.getInstance. This is what would be frowned upon. The approach mentioned in the question actually tackles all the issues that a singleton settings class would have introduced and is is therefore the right way forward.

That said, one thing I would change is that I would make the ObjValidatorFactory class depend on an ISettingsFactory interface rather than depending on ISettings directly. This would redirect the responsibility of creating an ISettings implementation from the client class to ObjValidatorFactory. 

class ObjValidatorFactory: IObjValidatorFactory
{
    private ISettingsFactory settingsFactory;

    public ObjValidatorFactory(ISettingsFactory settingsFactory)
    {
        this.settingsFactory = settingsFactory;
    }

    IObjValidator Create(ObjValidatorMode mode)
    {
        //source could be the file name for example
        ISettings settings = settingsFactory.Create(source);
        switch(mode)
        {
             case ObjValidatorMode.One:
                 return ObjValidator1();

             case ObjValidatorMode.Two:
                 return ObjValidator2(settings);
        }
    }


}

The factory could be instantiated as follows :

ISettingsFactory fileSettingsFactory = new FileSettingsFactory();
IObjValidatorFactory objectValidatorFactory = new ObjValidatorFactory(fileSettingsFactory);
objectValidatorFactory.Create(ObjValidatorMode.STRICT);

Upvotes: 1

Sunil Singhal
Sunil Singhal

Reputation: 603

Few observations: - Not all validators require ISettings - Object is created based on mode

In this case, I think that Abstract factory suits more, delegating responsibilities to child classes.

It is ok to have fields in Factory class which will be responsible for factoring the same object, think of a biscuit factory always creating a same biscuit.

The way, you have defined the factory class, can lead to content coupling. In future, you will start injecting more and more dependencies which will not be maintainable.

Another way could be to inject the pre-built validators as a map in the factory and on create() , get an entry from the map.

Upvotes: 0

Goose
Goose

Reputation: 546

I'd rather avoid that if I could, everything else being equal, if ObjValidator2 was somehow replaced with a different validator (e.g. ObjValidator2a),

A.) you would now have to change your constructor to accept a settings object for objValidator2a B.) drop it from the constructor because objValidator2a does not need settings for objValidator2 C.) keep the constructor as is knowing that the setting passed will never be used.

YMMV

Upvotes: 1

Related Questions