Feedback on automatic form filling with reflection

Question

Context:

I need to fill a handful of .docx templates with answers that come from form.responses, which is a dynamic object that comes from a JSON and can have different properties that vary according to the type of the form that was answered.

To do the filling, we need to instantiate a DTO called BindDto that takes the template string (eg.: ${PersonName}) and the value that will replace it (eg.: form.responses.client_name = "John Doe").

In my case, I created some [X]Constants classes that have properties that look like this...

public static readonly KeyValuePair<string, string> PERSON_NAME = new KeyValuePair<string, string>("client_name", "${PersonName}");
public static readonly KeyValuePair<string, string> PERSON_AGE = new KeyValuePair<string, string>("my_age", "${PersonAge}");
public static readonly KeyValuePair<string, string> MOTHER_NAME = new KeyValuePair<string, string>("mother", "${MotherName}");

...to map which template string each field from the JSON correponds to.

The code below was written so I could fill all the templates independent from which fields they might have by searching the [X]Constants classes and using the KeyValuePairs that I have set.

It works (o' course), but it seems I little... clumsy. And I don't have enough knowledge about C# to be sure this isn't pure madness.

What do you think about this code? Is it valid? Is is overengineering? How can I improve it?

I don't think performance will ever be a concern, but I would like to be enlightened in that sense too.

Thanks in advance to anyone willing to discuss it!

public List<BindDto> GenerateBinds(IEnumerable<Form> forms)
{
    var constants = new List<FieldInfo>();
    var binds = new List<BindDto>();

    constants.AddRange(typeof(AuthConsts).GetFields());
    constants.AddRange(typeof(ClientConsts).GetFields());
    constants.AddRange(typeof(ContactConsts).GetFields());
    constants.AddRange(typeof(AddressConsts).GetFields());
    constants.AddRange(typeof(ReferenceConsts).GetFields());

    foreach (var form in forms)
    {
        var formResponses = form.response.ToObject<Dictionary<string, string>>();

        IEnumerable<string> formFields = formResponses.Keys;

        foreach (var field in formFields)
        {
            foreach (var constant in constants)
            {
                if (constant.FieldType == typeof(KeyValuePair<string, string>))
                {
                    var templateString = (KeyValuePair<string, string>)constant.GetValue(this);

                    if (templateString.Key == field)
                        binds.Add(new BindDto(templateString.Value, formResponses[field]));
                }
            }
        }
    }

    return binds;
}

Answer 1

Just to tell you upfront, your code looks good, is easy to read and everything is well named. But it can be improved nevertheless.

First thing, public methods should always validate its method arguments. Passing null would lead to a nasty NullReferenceException at the start of the most outer foreach loop.

I would suggest making the constants a class level variable being filled in the constructor, because the types won't usually change while your code is running.

By restricting FieldType of the constants to KeyValuePair<string, string> will eliminate the if inside the most inner loop and will limit the constants to a smaller number.

Omitting braces {} although they may be optional can be dangerous because it may lead to hidden and therfor hard to find bugs. I would like to encourage you to use them.

Implementing the mentioned points will look like this

private readonly List<FieldInfo> constants = new List<FieldInfo>();
public ctor()
{
    constants.AddRange(typeof(AuthConsts).GetFields());
    constants.AddRange(typeof(ClientConsts).GetFields());
    constants.AddRange(typeof(ContactConsts).GetFields());
    constants.AddRange(typeof(AddressConsts).GetFields());
    constants.AddRange(typeof(ReferenceConsts).GetFields());
}

public List<BindDto> GenerateBinds(IEnumerable<Form> forms)
{
    if (forms == null) { throw new ArgumentNullException(nameof(forms)); }

    var neededType = typeof(KeyValuePair<string, string>);
    var binds = new List<BindDto>();
    var keyValueConstants = constants.Where(c => c.FieldType == neededType);

    foreach (var form in forms)
    {
        var formResponses = form.response.ToObject<Dictionary<string, string>>();

        IEnumerable<string> formFields = formResponses.Keys;

        foreach (var field in formFields)
        {
            foreach (var constant in keyValueConstants)
            {
                var templateString = (KeyValuePair<string, string>)constant.GetValue(this);

                if (templateString.Key == field)
                {
                    binds.Add(new BindDto(templateString.Value, formResponses[field]));
                }
            }
        }
    }

    return binds;
}

Answer 2

In addition of @Heslacher points, I would say the way you're handling the constants is making you doing extra work.

public static readonly KeyValuePair<string, string> PERSON_NAME = new KeyValuePair<string, string>("client_name", "${PersonName}");
public static readonly KeyValuePair<string, string> PERSON_AGE = new KeyValuePair<string, string>("my_age", "${PersonAge}");
public static readonly KeyValuePair<string, string> MOTHER_NAME = new KeyValuePair<string, string>("mother", "${MotherName}");

Using the above can be simplified in a Dictionary. So, you can combine them to this :

public static readonly Dictionary<string, string> Person_Info = new Dictionary<string, string>
{
    { "client_name", "${PersonName}" },
    { "my_age", "${PersonAge}" },
    { "mother", "${MotherName}" }
};

With this, you can get the constant value like this :

var key = "mother";

if(Person_Info.TryGetValue(key, out string _value))
{
    // do something with the value
}

So, if you go to each class in [X]Constants classes and combine the KeyValuePair<string, string> you'll flatten your classes, and might end up with one class that holds multiple Dictionary properties, which will make things even simpler to access, and If the keys are unique in all classes, you can combine them all in one dictionary, so you can have one method that will process all the bindings process.

public IEnumerable<BindDto> GenerateBinds(IEnumerable<Form> forms)
{
    // for demoenstration only when using Dictionary<string,string> .
    var constants = ConstsClass.ConstantsDictionary; 


    foreach (var form in forms)
    {
        var formResponses = form.response.ToObject<Dictionary<string, string>>();

        foreach (var field in formResponses.Keys)
        {
            if(constants.TryGetValue(field, out string _value))
            {       
                yield return new BindDto(_value, formResponses[field]);
            }               
        }
    }
}

With the Dictionary approach, you'll also eliminate the need of using reflection. If you need multiple dictionaries, you can adjust your method, and add a dictionary argument so you can pass the correct dictionary something like this :

public IEnumerable<BindDto> GenerateBinds(IEnumerable<Form> forms, IDictionary<string, string> constants)
{
    foreach (var form in forms)
    {
        var formResponses = form.response.ToObject<Dictionary<string, string>>();

        foreach (var field in formResponses.Keys)
        {
            if(constants.TryGetValue(field, out string _value))
            {       
                yield return new BindDto(_value, formResponses[field]);
            }               
        }
    }
}