Design pattern to get rid of switch/case from enum property

user_16960394 0
Viewed 193

I'm currently trying to refactor a method that receives a property name and set it's value:

private async Task < Enrichment > ParseEnrichmentNewDataAsync(int leadId, string property, string newValue) {
  var enrichment = await _context.EnrichmentRepository.GetByLeadIdAsync(leadId);
  if (enrichment == null) {
    enrichment = new Enrichment() {
      LeadId = leadId
    };
  }

  Enum.TryParse < PropertyType > (property, out
    var enumProperty);
  switch (enumProperty) {
  case PropertyType.Color:
    enrichment.Color = newValue;
    break;
  case PropertyType.UsedVehicleModel:
    enrichment.UsedModel = newValue;
    break;
  case PropertyType.UsedVehicleYear:
    enrichment.UsedYear = newValue;
    break;
  case PropertyType.UsedVehicleKm:
    enrichment.UsedKm = newValue;
    break;
  case PropertyType.Payment:
    enrichment.Payment = Convert.ToInt32(EnumExtension.GetEnumValueFromDescription < PaymentType > (newValue));
    break;
  case PropertyType.ScheduleDate:
    enrichment.ScheduleDate = DateTime.ParseExact(newValue, "dd/MM/yyyy", CultureInfo.InvariantCulture);
    break;
  case PropertyType.SchedulePeriod:
    enrichment.SchedulePeriod = newValue;
    break;
  case PropertyType.SchedulePhone:
    enrichment.SchedulePhone = newValue;
    break;
  case PropertyType.PurchaseType:
    enrichment.PurchaseType = newValue;
    break;
  case PropertyType.HasOptInNextJeep:
    enrichment.HasOptInNextJeep = Convert.ToBoolean(newValue);
    break;
  }

  return enrichment;
}

Using reflection would work for most of the fields, but some values need to be converted before being assigned. Is there a design pattern or a better way to improve this code?

refactoring design-patterns c#
2 Answers

Assuming PropertyType is a numeric 0-based enum, you could do this:

private static Action<Enrichment, string>[] _propertySetters = new Action<Enrichment, string>[]
{
    // Assuming Color = 0
    (enrichment, value) => enrichment.Color = value,
    // Assuming UsedVehicleYear = 1
    (enrichment, value) => enrichment.UsedVehicleYear = value,
    // Assuming UsedVehicleModel = 2
    (enrichment, value) => enrichment.UsedVehicleModel = value,
    // ...
};

private async Task<Enrichment> ParseEnrichmentNewDataAsync(int leadId, string property, string newValue)
{
     // ...
     Enum.TryParse<PropertyType>(property, out var enumProperty);
     var setter = _propertySetters[(int)enumProperty];
     setter(enrichment, newValue);
     // ...
}

You could also do this with a Dictionary<PropertyType, Action<Enrichment, string>> instead of a flat array, but if performance is what you're going for, this is unlikely to be better than using switch ... case until you get to very large numbers of properties.

user_44676700

Method should do only one thing. Function ParseEnrichmentNewDataAsync does three things:

  1. Gets or creates Enrichment object
  2. Converts the specified value if needed
  3. Sets property of Enrichment object

It would be better to somehow separate converting from property setting. I would suggest the EnrichmentPropertySetter class, which encapsulates property setting logic:

public abstract class EnrichmentPropertyActionBase
{
    public abstract void SetProperty(Enrichment enrichment, string value);
}

public class EnrichmentPropertyAction<T> : EnrichmentPropertyActionBase
{
    public Action<Enrichment, T> Set { get; set; }
    public Func<string, T> Convert { get; set; }

    public override void SetProperty(Enrichment enrichment, string value)
    {
        Set(enrichment, Convert(value));
    }
}

public class EnrichmentPropertySetter
{
    private readonly Dictionary<string, EnrichmentPropertyActionBase> _dictionary =
        new Dictionary<string, EnrichmentPropertyActionBase>();

    public EnrichmentPropertySetter()
    {
        _dictionary["Color"] = new EnrichmentPropertyAction<string>
        {
            Convert = s => s,
            Set = (e, s) => e.Color = s
        };
        ...
        _dictionary["Payment"] = new EnrichmentPropertyAction<int>
        {
            Convert = s => Convert.ToInt32(EnumExtension.GetEnumValueFromDescription<PaymentType>(s)),
            Set = (e, i) => e.Payment = i
        };
        _dictionary["ScheduleDate"] = new EnrichmentPropertyAction<DateTime>
        {
            Convert = s => DateTime.ParseExact(s, "dd/MM/yyyy", CultureInfo.InvariantCulture),
            Set = (e, d) => e.ScheduleDate = d
        };
        ...
        _dictionary["HasOptInNextJeep"] = new EnrichmentPropertyAction<bool>
        {
            Convert = s => Convert.ToBoolean(s),
            Set = (e, s) => e.HasOptInNextJeep = s
        };
    }

    public void SetProperty(Enrichment enrichment, string property, string value)
    {
        if (!_dictionary.TryGetValue(property, out EnrichmentPropertyActionBase action))
            throw new InvalidOperationException("Something goes wrong...");

        action.SetProperty(enrichment, value);
    }
}

Assuming that number of properties is limited, I don’t think using Dictionary may lead to significant performance issues.

But this solution is still not ideal, because we have to hardcode property names anyway. Maybe you should think about why you need to set properties by the specified name, and revisit the task.

user_83401670
Related