CODEWITHSUNDEEP
c#asp.netsolid-principles
Prisoner ZERO
Prisoner ZERO

Reputation: 14176

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.

Upvotes: 17

Views: 2593

Answers (3)

stromms
stromms

Reputation: 161

After about 6 months of experience in Dependency Injection, I've only discovered few cases where factories should set properties:

  1. If the setter is marked as internal, and the properties are expected to be set once by the factory only. This usually happens on interfaces with only getter properties whose implementations are expected to be created thru a factory.

  2. When the model uses property injection. I rarely see classes that use property injection (I also try to avoid building these), but when I do see one, and the needed service is only available elsewhere, it's a case where you have no choice.

For the bottom line, leave public setters out of factories. Only set properties that are marked as internal Let the clients decide on what properties they need to set if they are allowed to do so. This will keep your factories clean of unneeded functions.

Upvotes: 0

Roman Boiko
Roman Boiko

Reputation: 3586

Assuming that your factory interface is used from application code (as opposed to infrastructural Composition Root), it actually represents a Service Locator, which can be considered an anti-pattern with respect to Dependency Injection. See also Service Locator: roles vs. mechanics.

Note that code like the following:

var employee = Factory.Create<ExxonMobilEmployee, IEmployee>();

is just syntax sugar. It doesn't remove dependency on concrete ExxonMobilEmployee implementation.

You might also be interested in Weakly-typed versus Strongly-typed Message Channels and Message Dispatching without Service Location (those illustrate how such interfaces violate the SRP) and other publications by Mark Seemann.

Upvotes: 2

k.m
k.m

Reputation: 31464

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).

Upvotes: 8

Related Questions