Search code examples
c#asp.netsolid-principles

Should factories set model properties?


As part of an overall S.O.L.I.D. programming effort I created a factory interface & an abstract factory within a base framework API.

People are already starting to overload the factories Create method. The problem is people are overloading the Create method with model properties (and thereby expecting the factory to populate them).

In my opinion, property setting should not be done by the factory. Am I wrong?

public interface IFactory
{
    I Create<C, I>();
    I Create<C, I>(long id); //<--- I feel doing this is incorrect

    IFactoryTransformer Transformer { get; }
    IFactoryDataAccessor DataAccessor { get; }
    IFactoryValidator Validator { get; }
}

UPDATE - For those unfamiliar with SOLID principles, here are a few of them:

Single Responsibility Principle
It states that every object should have a single responsibility, and that responsibility should be entirely encapsulated by the class

Open/Closed Principle
The meaning of this principle is that when a get a request for a feature that needs to be added to your application, you should be able to handle it without modifying old classes, only by adding subclasses and new implementations.

Dependency Inversion Principle
It says that you should decouple your software modules. To achieve that you’d need to isolate dependencies.

Overall:
I'm 90% sure I know the answer. However, I would like some good discussion from people already using SOLID. Thank you for your valuable opinions.

UPDATE - So what do I think a a SOLID factory should do?

IMHO a SOLID factory serves-up appropriate object-instances...but does so in a manner that hides the complexity of object-instantiation. For example, if you have an Employee model...you would ask the factory to get you the appropriate one. The DataAccessorFactory would give you the correct data-access object, the ValidatorFactory would give you the correct validation object etc.

For example:

var employee = Factory.Create<ExxonMobilEmployee, IEmployee>();
var dataAccessorLdap = Factory.DataAccessor.Create<LDAP, IEmployee>();
var dataAccessorSqlServer = Factory.DataAccessor.Create<SqlServer, IEmployee>();
var validator = Factory.Validator.Create<ExxonMobilEmployee, IEmployee>();

Taking the example further we would...

var audit = new Framework.Audit(); // Or have the factory hand it to you
var result = new Framework.Result(); // Or have the factory hand it to you

// Save your AuditInfo
audit.username = 'prisonerzero';

// Get from LDAP (example only)
employee.Id = 10;
result = dataAccessorLdap.Get(employee, audit);
employee = result.Instance; // All operations use the same Result object

// Update model    
employee.FirstName = 'Scooby'
employee.LastName = 'Doo'

// Validate
result = validator.Validate(employee);

// Save to SQL
if(result.HasErrors)
     dataAccessorSqlServer.Add(employee, audit);

UPDATE - So why am I adamant about this separation?

I feel segregating responsibilities makes for smaller objects, smaller Unit Tests and it enhances reliability & maintenance. I recognize it does so at the cost of creating more objects...but that is what the SOLID Factory protects me from...it hides the complexity of gathering and instantiating said objects.


Solution

  • I'd say it's sticking to DRY principle, and as long as it's simple values wiring I don't see it being problem/violation. Instead of having

    var model = this.factory.Create();
    model.Id = 10;
    model.Name = "X20";
    

    scattered all around your code base, it's almost always better to have it in one place. Future contract changes, refactorings or new requirements will be much easier to handle.

    It's worth noting that if such object creation and then immediately properties setting is common, then that's a pattern your team has evolved and developers adding overloads is only a response to this fact (notably, a good one). Introducing an API to simplify this process is what should be done.

    And again, if it narrows down to simple assignments (like in your example) I wouldn't hesitate to keep the overloads, especially if it's something you notice often. When things get more complicated, it would be a sign of new patterns being discovered and perhaps then you should resort to other standard solutions (like the builder pattern for example).