3
\$\begingroup\$

The following classes are a simplification of an auto-generated code of an ORM (targeting Microsoft's Dynamics CRM):

public class Entity 
{
    public Dictionary<string, object> Fields { get; set;}
    public virtual T GetFieldValue<T>(string fieldName)
    {
         return (T)Fields[fieldName];
    }
    protected virtual void SetFieldValue(string fieldName, object value)
    {
         Fields[fieldName] = value;
    }
}
partial class Account : Entity
{
    public string Name
    get
    {
         return GetFieldValue<string>("Name");
    }
    set
    {
         SetFieldValue("Name", value);
    }
    public string Email
    get
    {
         return GetFieldValue<string>("Email");
    }
    set
    {
         SetFieldValue("Email", value);
    }
}

When an Entity's derived class is retrieved from the database, the base class Fields property is automatically assigned (again, this is not something in my control) with the selected fields.

I'd like a review of the following class, intended to validate "partially" filled objects:

public class EntitySubset<T> : Entity where T : Entity
{
    public EntitySubset(T subset)
    {
        foreach (var field in subset.Fields)
        {
            SetFieldValue(field.Key, field.Value);
        }
    }
    public override U GetFieldValue<U>(string fieldName)
    {
        if (!Fields.ContainsKey(fieldName))
        {
            throw new Exception($"{typeof(T).Name} must contain {fieldName} as a key!");
        }
        return base.GetFieldValue<U>(fieldName);
    }
}

an example usage:

private List<Account> GetSimiliarAccounts(EntitySubset<Account> targetAccount)
{
    var similarityFields = new[] { "Name", "Email" };
    var similiarAccounts = db.Where(account =>
       similarityFields.All(f => 
          account.GetFieldValue<object>(f) == targetAccount.GetFieldValue<object>(f))
    ).ToList();
    return similiarAccounts;
}

so that if GetSimiliarAccounts is called as follows:

GetSimiliarAccounts(new EntitySubset<Account>(new Account { Email = "b" }));

then the output is:

System.Exception: 'Account must contain Name as a key!'

Note that EntitySubset<T> and its error messaging is intended for developers to "fail fast", namely: to quickly spot and fix bugs stemming from a partially filled object.

What do you think of EntitySubset<T> as a validation implementation?

\$\endgroup\$
  • 1
    \$\begingroup\$ Good to see your question back up, and with sufficient context. \$\endgroup\$ – dfhwze Jul 31 at 21:05
  • 1
    \$\begingroup\$ Nothing ever is irrelevant around here :) \$\endgroup\$ – dfhwze Jul 31 at 21:15
2
\$\begingroup\$

Review

You should avoid hardcoded strings. Take advantage of nameof. Also, expression-bodied members (arrow notation) makes code cleaner to read.

public string Name
{
    get => GetFieldValue<string>(nameof(Name));
    set => SetFieldValue(nameof(Name), value);
}

EntitySubset<T>'s method GetFieldValue makes sure a clean (but technical) error message is thrown whenever an asked field is not available.

public override U GetFieldValue<U>(string fieldName)
{
    if (!Fields.ContainsKey(fieldName))
    {
        throw new Exception($"{typeof(T).Name} must contain {fieldName} as a key!");
    }
    return base.GetFieldValue<U>(fieldName);
}

However, don't fallback to this method to perform business rules checks. The business layer should check that mandatory fields are filled in, before mapping the entities to the data layer.

You should also optimize ContainsKey and base.GetFieldValue. You don't want to perform 2 lookups here. Implement something like a TryGetFieldValue.

\$\endgroup\$
  • \$\begingroup\$ "The business layer should check that mandatory fields are filled in": well, that's the purpose of EntitySubset<T>. Don't you consider it part of the business layer? \$\endgroup\$ – HelterSkelter Jul 31 at 21:27
  • \$\begingroup\$ Perhaps so, but it's very generic (unspecific). I would argue you need a layer on top with specific rules and error messages tailored to the end-user. EntitySubset is a last resort fallback (in my opinion). \$\endgroup\$ – dfhwze Jul 31 at 21:28
  • \$\begingroup\$ I should've clarified (and I'm going to edit it into the question) that EntitySubset<T> and its error messaging is intended for developers to "fail fast", not for users. Note that an alternative input parameter for GetSimiliarAccounts is Account. \$\endgroup\$ – HelterSkelter Jul 31 at 21:33
  • \$\begingroup\$ You may edit the question to stipulate it's a technical error. That re-enforces the need for additional business rule validation a layer higher up the stack. \$\endgroup\$ – dfhwze Jul 31 at 21:34
  • \$\begingroup\$ Can you give a concrete example for such additional business rule validation? I'm not sure what else is needed that is not already handled by EntitySubset<T>. \$\endgroup\$ – HelterSkelter Jul 31 at 21:36

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.