How can I refactor this method in C# with a design pattern? [closed]

Question

I have this method in C# that could be refactored. Should I use a design pattern? I see too much repetition and it gets worse if more conditionals are added.

Should I change it to a method call?

public void CreateOrUpdateReportDefinition(ReportGroupSubReport reportGroupSubReport, bool isNew, int report)
{

    if (report == 1)
    {
        var entity = _envyUnitOfWork.ReportDefinitions.GetById(reportGroupSubReport.Id) ?? new ReportDefinition();
        if (isNew)
            entity.SetNew();

        _envyUnitOfWork.ReportDefinitions.InsertOrUpdate(entity, true);
    }
    else if (report == 2)
    {
        var entity = _envyUnitOfWork.TraxReports.GetById(reportGroupSubReport.Id) ?? new TraxReport();

        if (isNew)
            entity.SetNew();

        _envyUnitOfWork.TraxReports.InsertOrUpdate(entity, true);

    }


    Mapper.Map(reportGroupSubReport, entity);
    _envyUnitOfWork.Commit();


}

UPDATE

Ok, I have 2 classes:

ReportDefinition which use ReportDefinitions and TraxReport which uses TraxReports.

So report is 1 if ReportDefinitions or 2 if TraxReports.

Answer 1

It looks like your entities are implementing some kind of interface (as they both have the SetNew() method. I'm not sure if ReportDefinitions and TraxReports are also implementing a common interface, but it looks like they could since they both have the InsertOrUpdate method with the same signature.

So, perhaps you could create a GetEntitySet method on your unit of work class, which takes a type and returns a set. Then, as you have indicated, create a common method that takes an entity type and does the processing in its implementation:

private static void InsertOrUpdate(Type entityType, int subReportId, bool isNew)
{
    var entity = _envyUnitOfWork.GetEntitySet(entityType).GetById(subReportId) ?? Activator.CreateInstance(entityType);

    if (isNew)
        entity.SetNew();

    _envyUnitOfWork.EntitySet<T>().InsertOrUpdate(entity, true);
}

Which you can use like this:

if (report == 1)
{
    InsertOrUpdate(typeof(ReportDefinition), reportGroupSubReport.Id, isNew);
}
else if (report == 2)
{
    InsertOrUpdate(typeof(TraxReport), reportGroupSubReport.Id, isNew);
}

By the way, it's also a good idea to name integers:

public const int ReportDefinitionTypeId = 1;
public const int TraxReportDefinitionTypeId = 2;

So the if statements would be a bit clearer:

if (report == ReportDefinitionTypeId)
// ...
if (report == TraxReportDefinitionTypeId)

Answer 2

Depending on the number of if conditions, you could use the Strategy Pattern instead.

Something like this...(NOTE: This is not working code!)

public interface ICreateOrUpdateReportDefinition
{
    void CreateOrUpdate(ReportGroupSubReport reportGroupSubReport, bool isNew);
    bool ShouldHandle(int report);
}

public class CreateOrUpdateFactory
{
    private ICreateOrUpdateReportDefinition[] _definitions;

    public CreateOrUpdateFactory(ICreateOrUpdateReportDefinition[] definitions)
    {
        _definitions = definitions;
    }

    public void CreateOrUpdate(ReportGroupSubReport reportGroupSubReport, bool isNew, int report)
    {
        var definition = _definitions.FirstOrDefault(x => x.ShouldHandle(report));
        if (definition != null)
            definition.CreateOrUpdate(reportGroupSubReport, isNew);
    }
}

public class CreateOrUpdateReportDefinitions : ICreateOrUpdateReportDefinition
{
    var _envyUnitOfWork;

    public CreateOrUpdateReportDefinitions(var envyUnitOfWork)
    {
        _envyUnitOfWork = envyUnitOfWork;
    }

    public void CreateorUpdate(ReportGroupSubReport reportGroupSubReport, bool isNew)
    {
        var entity = _envyUnitOfWork.ReportDefinitions.GetById(reportGroupSubReport.Id) ?? new ReportDefinition();
        if (isNew)
            entity.SetNew();

        _envyUnitOfWork.ReportDefinitions.InsertOrUpdate(entity, true);
    }

    public bool ShouldHandle(int report)
    {
        return report == 1;
    }
}

public class CreateOrUpdateTraxReports : ICreateOrUpdateReportDefinition
{   
    var _envyUnitOfWork;

    public CreateOrUpdateTraxReports(var envyUnitOfWork)
    {
        _envyUnitOfWork = envyUnitOfWork;
    }

    public void CreateorUpdate(ReportGroupSubReport repotGrouSubReport, bool isNew)
    {
        var entity = _envyUnitOfWork.TraxReports.GetById(reportGroupSubReport.Id) ?? new TraxReport();

        if (isNew)
            entity.SetNew();

        _envyUnitOfWork.TraxReports.InsertOrUpdate(entity, true);
    }

    public bool ShouldHandle(int report)
    {
        return report == 2;
    }
}

You would setup the factory like this...

var envyUnitOfWork = new EnvyUnitOfWork();  // or however you create an instance of this...
var factory = new CreateOrUpdateFactory(new CreateOrUpdateReportDefinitions(envyUnitOfWork), new CreateOrUpdateTraxReports(envyUnitOfWork));

Then all you have to do is call it like this...

factory.CreateOrUpdate(reportGroupSubReport, isNew, report);

Final thoughts:

I like the Strategy Pattern when you have a number of conditions because it segregates the code into separate classes, making it easier to maintain and unit test.

Another nice thing about this approach is when new report definitions are added you just have to add a new class and pass that new class into the factory constructor. Nothing else changes which you are less likely to break code elsewhere.

I tend to pass classes that do not change into the constructor which is why I am passing _envyUnitOfWork in the constructor. There are others who prefer to pass everything into the method. I think either way will work but wanted to give you the two schools of thought so you can decide for yourself.