2
\$\begingroup\$

In the last months I have written an application that, among other things, parses financial documents. This application has grown quite organically and I am not happy with that. I split the application into several libraries by functionality and intended to use simple DTOs. These DTOs will be created in the library responsible for the parsing and passed around into other libraries to work with (which I am already not too happy in terms of dependency).

One example of such a DTO is this (the fields are all nullable on purpose since I might not be able to fill them):

public class KidData
{
    public string ISIN { get; set; }
    public Uri DownloadLink { get; set; }
    public string WKN { get; set; }
    public string Emittent { get; set; }
    public string ProductName { get; set; }
    public int? SRI { get; set; }
    public string ScenarioCurrency { get; set; }
    public Scenario StressScenario { get; set; }
    public Scenario PessimisticScenario { get; set; }
    public Scenario MediumScenario { get; set; }
    public Scenario OptimisticScenario { get; set; }
    public string BaseValue { get; set; }
    public DateTime? CreationDate { get; set; }
    public float? AskPrice { get; set; }
    public DateTime? EmissionDate { get; set; }
    public DateTime? DateOfRepayment { get; set; }
    public float? Strike { get; set; }
    public string StrikeUnit { get; set; }

    public FinProductData Product { get; set; }
}

There is a class with extension methods for this DTO that fills the data by string values like this (this mapping is not trivial in all cases and may require calculations):

public static SetValue(this KidData data, string field, string value)
{
    switch(field)
    {
        case "ISIN":
            data.ISIN = value;
            break;
        case ...
    }
}

This seemed like the best approach to me to limit the visibility of the functionality to fill my data to this very library. However due to inheritance I was not able to keep this pattern for all my DTOs as you can see:

public abstract class FinProductData
{
    public string Name { get; set; }
    protected float? _faceValue;
    public abstract float? FaceValue { get; protected set; }

    private static Logger logger = LogManager.GetCurrentClassLogger();

    public virtual void SetValue(string field, string value)
    {
        switch (field)
        {
            case "Name":
                Name = value;
                break;
            case "FaceValue":
                FaceValue = StringOperations.ParseEuroCurrency(value);
                break;
            default:
                logger.Warn($"Unsupported descriptor ({field}), it will be ignored");
                break;
        }
    }
}

Here is an example of an overriding class:

public class ExpressCertificate : FinProductData
{
    public override float? FaceValue
    {
        get => _faceValue;
        protected set => _faceValue = value;
    }

    public float? RepayThreshold { get; set; }
    public List<EarlyRepayment> EarlyRepayments { get; set; } = new List<EarlyRepayment>();

    private static Logger logger = LogManager.GetCurrentClassLogger();

    public override void SetValue(string descriptor, IList<(string groupName, string newValue)> groups, KidData availableData)
    {
        switch (descriptor)
        {
            case "RepayThreshold":
                RepayThreshold = StringOperations.ParseEuroCurrency(groups.First().newValue);
                break;
            case "EarlyRepayment":
                EarlyRepayments = EarlyRepayments ?? new List<EarlyRepayment>();
                EarlyRepayments.Add(new EarlyRepayment(groups, availableData));
                break;
            case "CalculatedEarlyRepayments":
                EarlyRepayments = CalculateEarlyRepayments(groups);
                break;
            default:
                base.SetValue(descriptor, groups, availableData);
                break;
        }
    }

You will probably agree that this 'solution' is very ugly, but I don't know how to improve it. I thought about keeping this class and generating a true DTO from it, but I am not really sure if that's the way to go. I also thought about having some sort of factory to create my DTOs but there are dependencies between data entries and I can also not really guarantee a specific order of completion of my objects.

So my question is pretty much how to solve this specific question or improve the overall design.

EDIT:

Since I have received several answers on how to structure my SetValue function: This is not my main concern. I would really appreciate the a better solution here, but sadly field is not a one-to-one correspondence to the field names. My main concern is how I can separate my data from my functionality.

\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to Code Review. I have some difficulties understanding your needs. Why is SetValue virtual in FinProductData? Could you provide example where it gets overriden? I can only think about builders (class factory - they can have virtual methods) or something like type convertors (similar, used e.g. with PropertyGrid). \$\endgroup\$ – firda Aug 22 '18 at 13:10
  • \$\begingroup\$ Are these SetValue methods only called by parsing code, or are they used in different contexts as well? Are all fields really optional, or are some fields required for processing? \$\endgroup\$ – Pieter Witvoet Aug 22 '18 at 13:24
  • \$\begingroup\$ @firda I added an example of an overriding class \$\endgroup\$ – Jerome Reinländer Aug 22 '18 at 13:28
  • \$\begingroup\$ @PieterWitvoet They are only called by parsing code. There are fields that will be set later on, but they are set directly. \$\endgroup\$ – Jerome Reinländer Aug 22 '18 at 13:29
  • 2
    \$\begingroup\$ @JeromeReinländer: I was asking specifically to see the reason why you cannot separate it as extension method. Still cannot see the reason (and the signature is even different). Still I like Adriano's answer, quite what I had in mind (type converters, property descriptors / reflection). The alternative would be to create BuilderClass for each corresponding class, like a wrapper, with all the logic moved there. \$\endgroup\$ – firda Aug 22 '18 at 13:39
7
\$\begingroup\$

If performance aren't critical (measure, do not guess) then you can use some Reflection:

public virtual void SetValue(string field, string value)
{
    var property = GetType().GetProperty(field);
    property.SetValue(this, value);
}

This will handle all simple cases (1:1 name matching and no complex conversions). You can handle some more conversions explicitely converting from string to the target type:

property.SetValue(this,
    Convert.ChangeType(value, property.PropertyType, CultureInfo.InvariantCulture));

With this you probably covert most of trivial cases. For everything else (or for non 1:1 name mapping) you can use a dictionary:

const Dictionary<string, ValueConverter> _converters = ...;

Where the key is the input field name and ValueConverter is something like this:

sealed class ValueConverter
{
    public ValueConverter(string name, Func<string, object> converter)
    {
        OutputPropertyName = name;
        Converter = converter;
    }

    public string OutputPropertyName { get; }
    public Func<string, object> Converter { get; }
}

For example an hypotetical mapping between FaceValue property and Price field name:

{ "Price", new ValueConverter(nameof(FinProductData.FaceValue), StringOperations.ParseEuroCurrency) }

Note that I'm using nameof() instead of hard-coding the property name. Now you can first search in the dictionary:

public virtual void SetValue(string field, string value)
{
    if (_converters.TryGet(field, out var converter))
    {
        var property = GetType().GetProperty(converter.OutputPropertyName);
        property.SetValue(this, converter.Convert(value));
    }
    else
    {
        var property = GetType().GetProperty(field);
        if (property == null)
        {
            logger.Warn($"Unsupported descriptor ({field}), it will be ignored");
        }

        var convertedValue = Convert.ChangeType(value,
            property.PropertyType,
            CultureInfo.InvariantCulture));

        property.SetValue(this, convertedValue);

    }
}

How to provide additional conversion rules in derived classes? You have to add more rules (if default behavior isn't enough) to the conversions dictionary. I think it's then time to move this logic to a separate class because it's both reusable and outside FinProductData responsibilities. You may do:

  • Add more rules in the ctor (discouraged outside very simple cases, it's a waste of resources).
  • Remove virtual from SetValue() and delegate to a separate method (see later).
  • Use conventions and static mappers (declared as fields).

Two words about the second option:

Change if (_converters.TryGet(field, out var converter)) to accept a list instead of a dictionary and ValueConverter to hold both names:

var converter = ResolveConverters()
    .FirstOrDefault(x => x.Field.Equals(field, StringComparison.Ordinal));

if (converter != null)
{
    var property = GetType().GetProperty(converter.OutputPropertyName);
    property.SetValue(this, converter.Convert(value));
}
else
{
    // Same as before
}

Add this:

protected virtual IEnumerable<ValueConverter> ResolveConverters()
{
    return ...; // Return a static list
}

And in derived classes:

protected override IEnumerable<ValueConverter> ResolveConverters()
{
    return super.ResolveConverters().Concat(...);
}

With closures you can also use class properties, if you need to.


Note that we're messing with responsibilities, for more complex cases (or ever-changing code) you should have:

  • ValueConverter class which describes a single conversion, more or less as described before.
  • PropertyMapper class which performs this mapping (which, IMHO, shouldn't be part of data class responsibilities).
  • The list of conversions for each class might be specified with an attribute PropertyMappingAttribute.

You might then have something like this:

[PropertyMapping(typeof(FinProductDataMapper))]
abstract class FinProductData { ... }

[PropertyMapping(typeof(ExpressCertificateMapper))]
sealed class ExpressCertificate{ ... }

With PropertyMapper which simply goes (top to bottom) through the class hierarchy to find relevant mappings (in this way you may also handle weird cases with properties hidden with new).


If it's not a problem for your design you may even drop the conversion list and use attributes on your properties:

[PropertyMapping("Price", typeof(FaceValueConverter))]
public float FaceValue { get; set; }

Converter may even directly derive from TypeConverter, in this way you gain a reusable code snippet.


Which one is better? Hard to say. If you have few classes and few fields then I'd start with the simplest working solution then I'd move to more complex implementations when required. Note that property access may even be made pretty fast without Reflection (check what Dapper, for example, does).

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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