Refactoring Day 1 : Encapsulate Collection

In certain scenarios it is beneficial to not expose a full collection to consumers of a class. Some of these circumstances is when there is additional logic associated with adding/removing items from a collection. Because of this reason, it is a good idea to only expose the collection as something you can iterate over without modifying the collection. Let’s take a look at some code

   1: public class Order
   2: {
   3:     private List<OrderLine> _orderLines;
   4:  
   5:     public IEnumerable<OrderLine> OrderLines
   6:     {
   7:         get { return _orderLines; }
   8:     }
   9:  
  10:     public void AddOrderLine(OrderLine orderLine)
  11:     {
  12:         _orderTotal += orderLine.Total;
  13:         _orderLines.Add(orderLine);
  14:     }
  15:  
  16:     public void RemoveOrderLine(OrderLine orderLine)
  17:     {
  18:         orderLine = _orderLines.Find(o => o == orderLine);
  19:         if (orderLine == null) return;
  20:  
  21:         _orderTotal -= orderLine.Total
  22:         _orderLines.Remove(orderLine);
  23:     }
  24: }

 

As you can see, we have encapsulated the collection as to not expose the Add/Remove methods to consumers of this class. There is some other types in the .Net framework that will produce different behavior for encapsulating a collection such as ReadOnlyCollection but they do have different caveats with each. This is a very straightforward refactoring and one worth noting. Using this can ensure that consumers do not mis-use your collection and introduce bugs into the code.

 

This is Day 1 of the 31 Days of Refactoring. For a full list of Refactorings please see the original introductory post.

Related Articles:

Post Footer automatically generated by Add Post Footer Plugin for wordpress.

About Sean Chambers

I am a Senior software developer from Palm Coast, Florida. An advocate of Domain Driven Design, Behavior Driven Development, creator of FluentMigrator and community activist. I am married to my beautiful wife Erin and am the proud father of two wonderful children. I currently reside at ACI, a local insurance industry/mortgage software company that excels in creating solutions using Agile methodologies.
This entry was posted in Uncategorized. Bookmark the permalink. Follow any comments here with the RSS feed for this post.
  • http://meandmycode.com Stephen

    In your example you don’t really take much advantage, you expose two ‘destructive’ methods anyway and don’t control their use at all, what you do do is to use these to act as observation points.. I think the example would be better to expose ICollection and have its underlaying type an ObservableCollection so you can observe additions / removals privately.

    There are certainly good reasons to control access, and its almost always better to do it with componentization vs inheritance (imo), just perhaps not nec~ for the example scenario?

  • Roxin

    @Stephen – I don’t think you need to control their usage – what you need to control is : ” Every time an OrderLine is added or removed ensure that the orderTotal is recalculated .”

    This way you’ll never have to remember what you need to do when when adding an OrderLine.

  • http://meandmycode.com Stephen

    @Roxin, I disagree in that I feel there are two things to use this tactic.. policy (control) and observation.. what was demonstrated was observation, but it could have been done with an observable collection.. and kept the usage more simple:

    order.OrderLines.Add(item);

    vs

    order.AddOrderLine(item);

    Although this is a common pattern to use, I find it irritating and counter intuative, as well as putting more responsibility into the aggregate.. in the end, the reason it is done in this case is because the order wants to optimize the way it generates totals, but it doesn’t need to be explitely included in the loop, because it handles managing the lines collection it can privately observe without it being obvious it is doing so.

  • Graham

    Unfortunately there’s nothing stopping a consumer of this class from casting the IEnumerable back into a list, and having their wicked way with it (unlike a ReadOnlyCollection which actually wraps the list). This is basically security through obscurity, never a safe bet!

  • http://www.lostechies.com/members/schambers/default.aspx schambers

    @Graham

    Good point, but I don’t like the fact that ReadOnlyCollection throws exceptions. Using exceptions to manage control flow is never a safe bet either!

  • Roxin

    @Graham, you could return in the IEnumerable a new List(innerList) .

    :)

  • Erik

    I have the following extension method

    public static IEnumerable ProtectedReturn(this Enumerable enumerable)
    {
    foreach (var enumerable1 in enumerable)
    {
    yield return enumerable1;
    }
    }

    Then you can
    private IList _orderLines=new List();
    public IEnumerable
    OrderLines
    {
    get { return _orderLines.ProtectedReturn(); }

    }

    A resharper to insert these means you only type your type and Property Name to quickly Add them with no more typing than simply exposing the Collection.

    Addresses Grahams issues. Also you can create your same Add/Remove Template Methods in the Resharper template so all you have to type is the Type and optionally the Collection Property Name.

    something like below

    private IList< $TYPE$> _$PROPERTYL$s = new List< $TYPE$>();
    public IEnumerable< $TYPE$> $PROPERTY$s
    {
    get{return _$PROPERTYL$s.ProtectedReturn();}
    }
    public void Add$PROPERTY$($TYPE$ $PROPERTYL$)
    {
    _$PROPERTYL$s.Add($PROPERTYL$);
    }
    public void Remove$PROPERTY$($TYPE$ $PROPERTYL$)
    {
    _$PROPERTYL$s.Remove($PROPERTYL$);
    }

  • Erik

    Stephen,
    How is digging into an internal collection more simple? Which way is more agreeable to refactoring. If a policy constraint comes up which could accommodate it easier. Your way would require you to refactor all calling code. To me this is a much more Tell Don’t Ask interface than an observable collection. Also your interior IList could be provided by an ORM or other framework than may not provide observation points and correct me if I am wrong, but wouldn’t you also have to enumerate your collection on every collection changed notification. The simple act of adding a large list of items would force an enumeration on each add which can be costly by itself. When you consider that you could map your interior list as a bag(with NHibernate) this would also force you to bring in every collection item to simply add an item (not an issue for small collections but able to be avoided for large ones) which would not be necessary in Seans method if you mapped to a bag in your ORM.

    Thanks,

    Erik

  • http://meandmycode.com Stephen

    @Erik, theres no reason you can’t have a list implementation where you can inject policy.. the whole point being the caller doesn’t have to implicitely include the aggregate by calling it rather than the ‘collection’ you would normally with any old .net object.

    And theres no digging, the collection simply pushes observable events when items are removed or added.. and no you wouldn’t need to enumerate the list, you get an enumerable of added / removed items in the event args (to support mass adds / removes).

    Remembering the main reason these methods exist AddX vs X’s.Add is purely by a limitation that there weren’t great ways to add policy or observation in from the aggregate.. it isn’t something thats desirable..

  • Erik

    Stephen,

    Thanks I was not aware of what the events args were of OnCollectionChanged. That said I still find the contract of the aggregate to be much cleaner and more explicit with the Add Remove methods. In fact if this were in my Domain I would probably even new up the OrderLine Item as part of the method instead of having it passed in as an argument.

    My Aggregate might start like
    http://www.codepaste.net/kho2qm

    and then later evolve in many different directions including moving to a write only model or adding back references on your internal representation.

    http://www.codepaste.net/n1no9o

    It seems to me that this is a case where by being explicit in your contract, you can evolve many more directions while still maintaining the same external contract than what you gain by violating Dementer (I know there are often good reasons to violate Dementer, but I am not sure what they are here.) What is it we gain by exposing and observing the collection. I started down that path once and wrote a few list Decorators that provided On Insert, After Insert, etc… to Existing ILists.

    The problem is that decorating the list and wiring up your event handlers was not much if any less code and when your object evolved and you wanted to change your internal representation of the collection(like to a “write only model” like above), you had to change all your calling code or write a bunch of biderectional synchronization code and events.

    Often times 1 refactoring can lead to another. You might make the first refactoring above and then you think Why do my callers have to know how to construct an orderline. Let’s fix all the places I am newing up an orderline before this gets out of hand.

    The calling code goes from

    var order=OrderProvider.GetOrder(OrderID);
    var orderline = new orderline (Order,itemType,qty);
    order.Orderlines.Add(orderline);

    to

    OrderProvider.GetOrder(OrderID).AddItems(itemType,qty);

    and has fewer reasons to change. To me this is a good thing.

    Thanks,

    Erik

  • http://codeclimber.net.nz Simone

    Hey Sean,
    are you thinking about putting all the refactorings into a kind of eBook or at least providing them all into a PDF as the DDD series of post was done at the beginning of this year?
    It would be a great reference… expecially since I’ll be on vacation from tomorrow till almost the end of August, and I’ll miss them all :)
    Simo

  • JeromeQc

    I this case I would prefer to have another object to encapsulate access the collection and subscribe to its events instead of adding logic in the collection itself.

    It think this breaks the Single Responsibility Principle. I would rather have an OrderService to handle the business logic and add up the total. This way the order collection would be lighter to carry around or reuse on other layers. I personally don’t like to embed business logic into entities (or their collection) to keep the domain simpler.

    I like to see entities as simple documents an nothing more. But of course this approach is still good and I guess it is just a personal choice.

    Here’s a quote from Domain Driven Design Quickly: “The verbs of the language, associated with their corresponding nouns become the part of the behavior of those objects. But there are some actions in the domain, some verbs, which do not seem to belong to any object. They represent an important behavior of the domain, so they cannot be neglected or simply incorporated into some of the Entities or Value Objects. Adding such behavior to an object would spoil the object, making it stand for functionality which does not belong to it. Nonetheless, using an object-oriented language, we have to use an object for this purpose. We can’t just have a separate function on its own. It has to be attached to some object. Often this kind of behavior functions across several objects, perhaps of different classes. For example, to transfer money from one account to another; should that function be in the sending account or the receiving account? It feels just as misplaced in either.”

    I am not a design guru so I might be taking this principle a bit too far. What do you think about it?

    I really enjoy your posts, I came across your blog before and I think you are a good good writer. I will follow this series for sure!

  • JeromeQc

    I this case I would prefer to have another object to encapsulate access the collection and subscribe to its events instead of adding logic in the collection itself.

    It think this breaks the Single Responsibility Principle. I would rather have an OrderService to handle the business logic and add up the total. This way the order collection would be lighter to carry around or reuse on other layers. I personally don’t like to embed business logic into entities (or their collection) to keep the domain simpler.

    I like to see entities as simple documents an nothing more. But of course this approach is still good and I guess it is just a personal choice.

    Here’s a quote from Domain Driven Design Quickly: “The verbs of the language, associated with their corresponding nouns become the part of the behavior of those objects. But there are some actions in the domain, some verbs, which do not seem to belong to any object. They represent an important behavior of the domain, so they cannot be neglected or simply incorporated into some of the Entities or Value Objects. Adding such behavior to an object would spoil the object, making it stand for functionality which does not belong to it. Nonetheless, using an object-oriented language, we have to use an object for this purpose. We can’t just have a separate function on its own. It has to be attached to some object. Often this kind of behavior functions across several objects, perhaps of different classes. For example, to transfer money from one account to another; should that function be in the sending account or the receiving account? It feels just as misplaced in either.”

    I am not a design guru so I might be taking this principle a bit too far. What do you think about it?

    I really enjoy your posts, I came across your blog before and I think you are a good good writer. I will follow this series for sure!

  • JeromeQc

    Oups, sorry for the duplicate, this is against the DRY principle ; )

  • http://hubpages.com/hub/Membership-Blueprints-Review-- Josh

    I remember when I first wanted to limit functionality of a list and being dismayed at having to write such additional code, but such is life. Thanks for the clear example.

  • http://www.xwaveradio.com JeromeQc

    I came around to see if I got a reply and I realized the order total is not really encapsulated because any imprudent developer reusing this object, not aware of the implementation, could decide to move the lines in some other data structure that is more handy and the order total wouldn’t add up anymore. I think implementing INotifyPropertyChanged would be a much better approach.

    Here’s the test I wrote just to make sure I wasn’t in the threes:

    [TestFixture]
    public class OrderTests
    {
    [Test]
    public void OrderTotalIsNotEncapsulated()
    {
    var order = new Order();
    var orderLines = new List(order.OrderLines);

    orderLines.Add(new OrderLine(1));
    Assert.AreEqual(0, order.Total);
    }
    }

    class Order
    {
    private List _orderLines = new List();
    private double _orderTotal;

    public IEnumerable OrderLines
    {
    get { return _orderLines; }
    }

    public double Total { get { return _orderTotal; } }

    public void AddOrderLine(OrderLine orderLine)
    {
    _orderTotal += orderLine.Total;
    _orderLines.Add(orderLine);
    }

    public void RemoveOrderLine(OrderLine orderLine)
    {
    orderLine = _orderLines.Find(o => o == orderLine);

    if (orderLine == null)
    return;

    _orderTotal -= orderLine.Total;
    _orderLines.Remove(orderLine);
    }
    }

    class OrderLine
    {
    public OrderLine(double total)
    {
    Total = total;
    }

    public double Total { get; private set; }
    }

  • http://azurecoding.net mbrown77@gmail.com

    The trick to preventing the user from casting back to a List is to just explicitly return an enumerator

    public IEnumerable OrderLines
    {
    get { return _orderLines.Select(o=>o); }
    }

    Will return only the IEnumerable and casting will return an InvalidCastException. Also, you get the advantage of deferred enumeration. Loading the Enumerable into a ReadOnlyCollection, immediately iterates over the entire list.

  • zhang hua

    it is very important to me thanks my english is very poor.