Open-closed Principle - How to refactor with arguments to providers

Question

I'm working on some legacy dotnet framework 4.7 code which seems to break the open-closed principle. The report classes do pretty much the same thing: Gets some items from a database, runs some transformation and returns them in the GetItems method. I was asked to add another report, but this would mean changing the ReportManager class which just grows and grows. The reports need different parameters, which makes it difficult for me to see how I can move forward. How can I refactor this so it doesn't break the O in SOLID?

UPDATED

Added missing call to GetItems() in both reports, as well as calls to _repository.


public interface IReport
{
    string GetItems();
}

public class StoreItemsReport : IReport
{
    private readonly int[] _numbers;
    private readonly IRepository _repository;
    public StoreItemsReport(IRepository repo, int[] numbers)
    {
        this._repository = repo;
        this._numbers = numbers;
    }
    public string GetItems()
    {
        return _repository.GetStoreItems(numbers);
    }
}

public class OnlineItemsReport : IReport
{
    private readonly string _account;
    private readonly IRepository _repository;
    public OnlineItemsReport(IRepository repo, string account)
    {
        this._repository = repo;
        this._account = account;
    }
    public string GetItems()
    {
        return _repository.GetOnlineItems(account);
    }
}

public class ReportManager
{
    private readonly IRepository repository;
    public ReportManager(IRepository repository)
    {
        this.repository = repository;
    }
    public void HandleSupplierItems(int[] numbers)
    {
        var items = new StoreItemsReport(repository, numbers).GetItems();
        Utils.SendData("storeitems-url", items);
        Emailer.SendReport(items);
    }

    public void HandleStoreItems(string account)
    {
        var items = new OnlineItemsReport(repository, account).GetItems();
        Utils.SendData("onlineitems-url", items);
        Emailer.SendReport(items);
    }

    // and so on
}

// I would like to do something like this, but how do I add arguments to the providers
var provider = providerFactory.GetProvider("Suppliers");
var items = provider.GetItems();
Utils.SendData("url", items);
Emailer.SendReport(items);

Yes, but I can't see how I can factory this. I have tried this factory method: ```public static T CreateInstance(params object[] constructorArguments) where T : class, IReport { return (T)Activator.CreateInstance(typeof(T), constructorArguments); }``` — M Raymaker, Mar 02 '21 at 13:54

Johnathan Barclay · Accepted Answer · 2021-03-05T09:25:14.417

1

You could have your ReportManager accept a factory method:

public class ReportManager
{
    private readonly IRepository repository;
    public ReportManager(IRepository repository)
    {
        this.repository = repository;
    }

    public void HandleItems(Func<IRepository, IReport> factory, string url)
    {
        var items = factory(repository).GetItems();
        Utils.SendData(url, items);
        Emailer.SendReport(items);
    }
}

Which could be used like this:

var numbers = new[] { 1, 2, 3 };
reportManager.HandleItems(repo => new StoreItemsReport(repo, numbers), "storeitems-url");

edited Mar 05 '21 at 09:25

answered Mar 02 '21 at 13:44

Johnathan Barclay

18,599
1
22
35

Brilliant, this was exactly what I was looking for. Clean and to the point. Thank you. – M Raymaker Mar 02 '21 at 13:53
Just a minor correction: var items = factory(repository).GetItems(); – M Raymaker Mar 02 '21 at 14:51
@MRaymaker That contradicts the existing code, where `GetItems` is not called within `ReportManager`. Should it be? – Johnathan Barclay Mar 02 '21 at 14:54
Yes, my bad. Have corrected the code now. – M Raymaker Mar 05 '21 at 06:40

SomeBody · Answer 2 · 2021-03-05T07:10:28.050

If you make the IRepository a public property of the IReport interface, you can just give a new instance of IReportto your ReportManager:

public interface IReport
{
    string GetItems();
    IRepository Repository {get;set;}
    string InfoString {get;}
}

public class StoreItemsReport : IReport
{
    private readonly int[] _numbers;
    public IRepository Repository {get;set;}
    public InfoString => "storeitems-url";
    public StoreItemsReport(int[] numbers)
    {
        this._numbers = numbers;
    }
    public string GetItems()
    {
        return Repository.GetOnlineItems(account);
    }
}

public class OnlineItemsReport : IReport
{
    private readonly string _account;
    public IRepository Repository {get;set;}
    public InfoString => "onlineitems-url";
    public OnlineItemsReport(string account)
    {
        this._account = account;
    }
    public string GetItems()
    {
        return Repository.GetStoreItems(numbers);
    }
}

public class ReportManager
{
    private readonly IRepository repository;
    public ReportManager(IRepository repository)
    {
        this.repository = repository;
    }
    public void HandleItems(IReport report)
    {
        report.Repository = repository;
        var items = report.GetItems();
        Utils.SendData(report.InfoString, items);
        Emailer.SendReport(items);
    }
}

Usage would be

var report = new OnlineItemsReport("test");
reportManager.HandleItems(report);

My bad, but I forgot to add the GetItems() part of the reports. When calling a report we need to query a specific method in the repository. So I can't just set the Repository as a property, because we need to call eg. _repository.GetStoreItems() — M Raymaker, Mar 05 '21 at 06:43
That shouldn't be a problem, see my updated answer. You set the value of the Repository property before you call GetItems(), to make sure that your property isn't null. — SomeBody, Mar 05 '21 at 07:11

Open-closed Principle - How to refactor with arguments to providers

2 Answers2