Sean Blakemore's Blog

Like trying to fit a square peg in a round hole

12. March 2010 13:18
by Sean
2 Comments

Bidirectional Integrity: IEnumerable and read only Entity properties

12. March 2010 13:18 by Sean | 2 Comments

Jimmy Bogard over at LosTechies has a great little series of posts going ahead at the moment on “strengthening your domain”. For the time constrained amongst us, I’ll quickly summarise his points so you can understand what I say here without going back to the source, however please do go ahead and check them out if your haven’t, there is some great advice there:

It’s his most recent post in the series on encapsulated collections which I’d like to talk more about.

Entity relationships and invariants

To quickly summarise Jimmy’s post so we can continue the discussion where he left off, let’s look at some poor code…

public class Customer
{
    public string FirstName { get; set; }
    public string LastName { get; set; }
    public string Province { get; set; }
    public List<Order> Orders { get; set; }

    public string GetFullName()
    {
        return LastName + ", " + FirstName;
    }
}

public class Order
{
    public Order(Customer customer)
    {
        Customer = customer;
        customer.Orders.Add(this);
    }

    public Customer Customer { get; set; }
}

What’s wrong with this?

  • First of all we’re exposing a setter on the Orders property of Customer, exposing a setter on a collection property is bad because it allows calling code to completely replace our collection from under our nose without us knowing.
  • We’re exposing our concrete List directly, this should be at the very least an IList so we can change our implementation without changing our interface.
  • It’s clear from the Order constructor that we have a bidirectional relationship here, the invariant is then that when the Customer property of the Order class is set, that Order should be in the Orders collection on the Customer class. Exposing the setter on the Customer property of the Order class and exposing in the Orders property of the Customer class a collection which can be modified allows people to mess with our invariant.

How can we fix it? Here’s Jimmy’s code…

public class Customer
{
    private readonly IList<Order> _orders = new List<Order>();

    public string FirstName { get; set; }
    public string LastName { get; set; }
    public string Province { get; set; }
    public IEnumerable<Order> Orders { get { return _orders; } }

    public string GetFullName()
    {
        return LastName + ", " + FirstName;
    }

    internal void AddOrder(Order order)
    {
        _orders.Add(order);
    }
}

public class Order
{
    public Customer Customer { get; protected set; }

    public Order(Customer customer)
    {
        Customer = customer;
        customer.AddOrder(this);
    }
}

Great, so we’ve removed the setters and now we’re only exposing an IEnumerable collection, no one can mess with our invariant and break our behaviour by not understanding our intention!

Looking a little closer

There is another interesting invariant at play in Jimmy’s code above, one of the invariants of the Order class is that it does not make sense to have an Order that is not associated with a Customer. To force the point, Jimmy has made it such that the only way to create an Order is by passing it a Customer as required by the constructor. The Order constructor then in turn calls an internal method on the Customer class to enforce the integrity of our bidirectional relationship. The method is left internal because we don’t want it to be seen everywhere, Orders and Customers should only become related in any way through the Order constructor.

Let’s make things slightly more complex. We’ve had a chat with our domain experts and they’ve told us that our Order which cannot exist without a Customer should also not exist without OrderLines. Ok that’s no problem, we know how to handle that, just more constructor parameters and internal methods!

This is where I start to have a problem, because although our external API is nice and clean, internally our code starts getting harder to understand and bigger in size as we add internal methods here and there.

Extension methods, generics, lambdas and reflection, what else is there in life?

Wouldn’t it be nice if we could add an item to an IEnumerable from another related entity without needing internal methods? Wouldn’t it be nice to be able to set protected properties? Well let’s be gratuitous and slightly immature and find a way to answer these questions using all the cool language features of the day!

Because we don’t want to expose dangerous functionality like this all throughout our application, really its just to help us make a clean API which is implemented cleanly on the inside too, we’re going to stuff everything in our Layer Supertype, you very likely have one for your domain anyway…

protected void SetInaccessibleProperty<TObj, TValue>(TObj target, TValue value,
    Expression<Func<TObj, TValue>> propertyExpression)
{
    propertyExpression.ToPropertyInfo().SetValue(target, value, null);
}

protected TValue GetInaccessibleProperty<TObj, TValue>(TObj target,
    Expression<Func<TObj, TValue>> propertyExpression)
{
    return (TValue)propertyExpression.ToPropertyInfo().GetValue(target, null);
}

protected void AddToIEnumerable<TEntity, TValue>(TEntity target, TValue value,
    Expression<Func<TEntity, IEnumerable<TValue>>> propertyExpression)
{
    IEnumerable<TValue> enumerable = GetInaccessibleProperty(target, propertyExpression);

    if (enumerable is ICollection<TValue>)
        ((ICollection<TValue>)enumerable).Add(value);
    else
        throw new ArgumentException(
            string.Format("Property must be assignable to ICollection<{0}>", typeof(TValue).Name));
}

protected void RemoveFromIEnumerable<TEntity, TValue>(TEntity target, TValue value,
    Expression<Func<TEntity, IEnumerable<TValue>>> propertyExpression)
{
    IEnumerable<TValue> enumerable = GetInaccessibleProperty(target, propertyExpression);

    if (enumerable is ICollection<TValue>)
        ((ICollection<TValue>)enumerable).Remove(value);
    else
        throw new ArgumentException(string.Format("Property must be assignable to ICollection<{0}>",
            typeof(TValue).Name));
}

Yikes, try saying that piece of generic soup five times really quickly after a few beers! But lets go a little further now and use these four methods to create something even more useful…

protected void AddManyToOne<TOne, TMany>(
    TOne one, Expression<Func<TOne, IEnumerable<TMany>>> collectionExpression,
    TMany many, Expression<Func<TMany, TOne>> propertyExpression)
{
    AddToIEnumerable(one, many, collectionExpression);
    SetInaccessibleProperty(many, one, propertyExpression);
}

protected void RemoveManyToOne<TOne, TMany>(
    TOne one, Expression<Func<TOne, IEnumerable<TMany>>> collectionExpression,
    TMany many, Expression<Func<TMany, TOne>> propertyExpression)
    where TOne : class
{
    RemoveFromIEnumerable(one, many, collectionExpression);
    SetInaccessibleProperty(many, null, propertyExpression);
}

protected void RemoveManyToMany<T1, T2>(
    T1 entity1, Expression<Func<T1, IEnumerable<T2>>> expression1,
    T2 entity2, Expression<Func<T2, IEnumerable<T1>>> expression2)
{
    RemoveFromIEnumerable(entity1, entity2, expression1);
    RemoveFromIEnumerable(entity2, entity1, expression2);
}

protected void AddManyToMany<T1, T2>(
    T1 entity1, Expression<Func<T1, IEnumerable<T2>>> expression1,
    T2 entity2, Expression<Func<T2, IEnumerable<T1>>> expression2)
{
    AddToIEnumerable(entity1, entity2, expression1);
    AddToIEnumerable(entity2, entity1, expression2);
}

Note that these are all protected so that they can only be accessed by derived types. The obvious exception being other idiots reflecting on your code! There is one other piece that’s missing, you may have spotted it already, I pulled it out into an extension method because I find it so useful elsewhere…

public static class ExpressionExtensions
{
    public static PropertyInfo ToPropertyInfo(this LambdaExpression expression)
    {
        var prop = expression.Body as MemberExpression;

        if (prop != null)
        {
            var info = prop.Member as PropertyInfo;
            if (info != null)
                return info;
        }

        throw new ArgumentException("The expression target is not a Property");
    }
}

Now we can take a look at how this would leave our Order and Customer class looking…

public class Customer : DomainBase
{
    public string FirstName { get; set; }
    public string LastName { get; set; }
    public string Province { get; set; }
    public IEnumerable<Order> Orders { get; protected set; }

    public string GetFullName()
    {
        return LastName + ", " + FirstName;
    }
}

public class Order : DomainBase
{
    public Customer Customer { get; protected set; }

    public Order(Customer customer)
    {
        AddManyToOne(customer, x => x.Orders, this, x => x.Customer);
    }
}

This looks much cleaner and easier to read, and the effect on a larger domain would be dramatic. Comments or criticisms please?

Performance considerations

Yes it’s slower and your application may associate Orders and Customers several orders of magnitude more slowly. What you need to figure out is whether that matters in any way. If associating an Order with a Customer truly is a bottleneck in your application then the only thing I can do is offer my congratulations on how amazing the rest of your code must be and also how many orders per second you must be processing!

Hope this helps!

Comments (2) -

Looks nifty, but I don't actually find the example particularly readable - and I quite enjoy both DDD and lambdas.  IMHO, if you were given the line items scenario as an invariant, you'd be better off using a builder pattern and/or a small DSL to guide proper construction of your instance.

Hi Remi,

Perhaps you're right, although for me personally "AddManyToOne" is about as explicit as it can get.

It doesn't really matter how you decide to create your graph, builder, DSL or anything else you choose. You still need a way to setup and maintain the integrity of your relationships without exposing setters or methods which allow consumers to break your invariants. Builder pattern or a DSL will not solve that, although either of them would layer nicely on this code presented.

Pingbacks and trackbacks (4)+

Comments are closed