Factories to create same objects with different data sources

Question

I have several handlers classes that implements same interface and factories to create handlers.

Handlers:

public class Handler1 : IHandler
{
 private readonly IService1 _service1;
 private readonly IService2 _service2;
 private readonly Handler1Data _data;
 public Handler1(IService1 service1, IService2 service2, Handler1Data data)
 {
 _service1 = service1;
 _service2 = service2;
 _data = data;
 }
 public async Task HandleAsync()
 {
 // await handle
 }
}
public interface IHandler
{
 Task HandleAsync();
}
public class Handler2 : IHandler
{
 private readonly IService3 _service3;
 private readonly IService4 _service4;
 private readonly Handler2Data _data;
 public Handler2(IService3 service3, IService4 service4, Handler2Data data)
 {
 _service3 = service3;
 _service4 = service4;
 _data = data;
 }
 public async Task HandleAsync()
 {
 // await handle
 }
}

Factories:

public interface IHandlerFactory
{
 Task<IHandler> CreateHandlerAsync();
 IHandlerFactory Map<TData>(TData data); 
}
public class Handler1Factory : IHandlerFactory
{
 private readonly IRepository1 _repository1;
 private readonly IService1 _service1;
 private readonly IService2 _service2;
 private Handler1FactoryData _data;
 public Handler1Factory(IRepository1 repository1, IService1 service1, IService2 service2)
 {
 _repository1 = repository1;
 _service1 = service1;
 _service2 = service2;
 }
 public async Task<IHandler> CreateHandlerAsync()
 {
 var handlerData = await _repository1.FindAsync(_data.SomeProperty1, _data.SomeProperty2);
 return new Handler1(_service1, _service2, handlerData);
 }
 public IHandlerFactory Map<TData>(TData data)
 {
 this._data = data as Handler1FactoryData;
 return this;
 }
}

I inject my factories with my DI contrainer. It worked several months, but now I need to extend logic. My factories work for one group of users only. Now I need to make work them for second group. What I found I don't need to make another group of handlers because the only difference is method to get my _data property. So, if I extend current system of classes I will have something like that:

public class FirstUsersGroupHandler1Factory : IHandlerFactory
{
 private readonly IFirstGroupRepository _repository1;
 private HandlerFactoryData _data;
 // inject same service for both user groups
 public FirstGroupHandler1Factory(IFirstGroupRepository repository1)
 {
 var handlerData = await _repository1.FindAsync(_data.SomeProperty1, _data.SomeProperty2);
 return new Handler1(_service1, _service2, handlerData); 
 }
 // mapping logic
}
 public class SecondUsersGroupHandler1Factory : IHandlerFactory
 {
 private readonly ISecondGroupRepository _repository2;
 private HandlerFactoryData _data;
 // inject same service for both user groups
 public SecondUsersGroupHandler1Factory (ISecondGroupRepository repository2)
 {
 var handlerData = await _repository2.FindAsync(_data.SomeProperty1, _data.SomeProperty2);
 return new Handler1(_service1, _service2, handlerData); 
 }
 // mapping logic
 }

In total I have now 10 handlers and factory for each handler. If I continue working I will have 10 handlers and 20 factories that do almost same thing, with the only different data source.

Questions:

1. Is it a potential problem to have different factories that create same objects?
2. What solution could you provide to solve duplicate factories problem?

• Laiv: 1) It should not. I suggest reading about the Abstract Factory pattern. 2) You are missing the FactoryDataSource abstraction. It's the very same case than HandlerFactoryData. Isn't it?
• Alex Gurskiy: @Laiv, what I need is to pass data (like integer id or complex object) somewhere (factory in my scenario) and then call repositories or another data source, get result, make object that I pass to concrete handler. The problem is I have similar scenario for different user groups, but each user group has there own data source. For example for trade user I'm going to call TradeUserRepository, for regular user - RegularUserRepository and so on. But handlers and data for them (let's say DTO models) are absolutely same. Repositories in my case are different depending on user group.
• Alex Gurskiy: @Laiv, so I see you suggest to make abstract factory, create there data for handler and handler right? Maybe I got you wrong, could you please provide few lines of code for me? Thanks for your time.
• Alex Gurskiy: @Laiv, your idea is to make abstract parent class, for example, put there AbstractUserGroupRepository, make child for each user group that resolve data source implementation. Correct or I missed something again?
• Laiv: More or less. I said abstract to illustrate. It could be any other technique to hide implementation details: interface segregation for instance. Right know you have a brittle abstraction and a lot of concrete classes. I think you are missing some more abstraction in that factories "engine". I will try to type an answer when I get home.

Answer 1

I completely changed my previous answer and give you better solution. Because as I see, you create redundant interface and methods. Thus, there are also redundant classes.

First, your FirstUsersGroupHandler1Factory and SecondUsersGroupHandler1Factory classes constructors try to return something. This code doesn't work.

Second, you need to use generics with your interface to avoid redundant concrete classes.

public interface IHandler
{
 Task HandleAsync();
}
public interface IHandlerFactory<T> where T: IHandler
{
 Task<T> CreateHandlerAsync();
 //IHandlerFactory Map<TData>(TData data); // don't use this.
}

Also, your IHandlerFactory Map<TData>(TData data) method is meaningless. Because it is returning its existing interface and interface can not initialize like concrete classes. If you want to use this method, you should have a class which implements IHandlerFactory<T> and if the class already implements interface, you can return this concrete class. No need that method.

The reason you create this method to ensure to set TData, then do this from class constructor like the others.

Third,

What I found I don't need to make another group of handlers because the only difference is method to get my _data property.

If so, you can generalize your factory class like using abstract class:

public abstract class HandlerFactory<T, R> : IHandlerFactory<T> where T : IHandler where R : class 
{
 private T _handler;
 private readonly IRepository<R> _repository;
 private readonly BaseHandlerFactoryData _data;
 public HandlerFactory(IRepository<R> repository, BaseHandlerFactoryData data)
 {
 _repository = repository;
 _data = data;
 }
 public abstract Task<T> CreateHandlerAsync();
}

If there is only difference is your _data, then give getting HandlerData responsibility to repository. You can create a method that need _data and return required handlerData.

public class BaseHandlerFactoryData
{
 public string SomeProperty1 { get; set; }
 public string SomeProperty2 { get; set; }
}
public interface IRepository<R> where R: class
{
 R FindAsync(BaseHandlerFactoryData data);
 // other repository methods here.
}
public class Repository1 : IRepository<Handler1Data>
{
 public Handler1Data FindAsync(BaseHandlerFactoryData data)
 {
 return context1.FindAsync(data.SomeProperty1, data.SomeProperty2);
 }
 //...
}
public class Repository2 : IRepository<Handler2Data>
{
 public Handler2Data FindAsync(BaseHandlerFactoryData data)
 {
 return context2.FindAsync(data.SomeProperty1, data.SomeProperty2);
 }
 //..
}

• Alex Gurskiy: I add comment above that explains the problem. Also what I don't like here is switch-case statement. I always try to avoid usage of if-else in details layer. In my world only "top" layer can use logical statements.
• Engineert: In real world, top layer may change and restricting yourself by doing everything for ideal world would hurt. This logical statements doesn't make your architecture poor. You can do this when you need to extend your feature. Besides, sometimes you need to read data from DB or service and manage and decide something by considering this data. Making caller as responsible to determine this situation is not always proper way.
• Engineert: @AlexGurskiy check this completely different answer.