-5
\$\begingroup\$

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.

\$\endgroup\$

closed as unclear what you're asking by Mast, SirPython, Vogel612, RobH, Ethan Bierlein Mar 29 '16 at 17:54

Please clarify your specific problem or add additional details to highlight exactly what you need. As it's currently written, it’s hard to tell exactly what you're asking. See the How to Ask page for help clarifying this question. If this question can be reworded to fit the rules in the help center, please edit the question.

  • \$\begingroup\$ I'm not sure about any design pattern in that case, I'd change the ifs for a switch statement and work with the calls to TraxReports, ReportDefinitions, etc as separated methods \$\endgroup\$ – Gerep Mar 28 '16 at 20:56
  • 1
    \$\begingroup\$ Would replace the report parameter with an Enum first. Then see if both objects obtained by ID have a common denominator that supports all methods afterwards. And then you can start your refactoring \$\endgroup\$ – Kolky Mar 28 '16 at 21:12
  • 2
    \$\begingroup\$ As we all want to make our code more efficient or improve it in one way or another, try to write a title that summarizes what your code does, not what you want to get out of a review. \$\endgroup\$ – Jamal Mar 28 '16 at 21:18
  • 4
    \$\begingroup\$ @JosephAckerman there were four comments as to why... we can't really review your code if we don't know what it is or does \$\endgroup\$ – Quill Mar 29 '16 at 4:27
  • 2
    \$\begingroup\$ Getting code to review with no comments is disheartening. I'm not inclined to figure out what code is supposed to do - I'm willing to ponder how to solve something naturally/readably, elegantly, efficiently, … (That said, I'm all with requiring clues how to do better, but not with not taking any clues.) \$\endgroup\$ – greybeard Mar 29 '16 at 7:07
-1
\$\begingroup\$

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)
\$\endgroup\$
  • 3
    \$\begingroup\$ The last example of integers is what you use an enum for... \$\endgroup\$ – Quill Mar 29 '16 at 4:27
  • \$\begingroup\$ thx I appreciate it \$\endgroup\$ – Joseph Ackerman Mar 29 '16 at 7:40
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Strategy pattern is a good suggestion IMO but I would recommend making the UoW a parameter to the method so the factory can be created once and used across multiple transactions \$\endgroup\$ – RobH Mar 29 '16 at 15:37

Not the answer you're looking for? Browse other questions tagged or ask your own question.