Obvious Testing or: How I Learned to Stop Using the new keyword

Here’s a basic test; looks simple right? Well, that’s because it is. A new Order is given three different order items, each with a quantity of one. We assert (using nunit) that there are three OrderItems, then RemoveAllItems and assert that there are zero.

        [Test]
        public void An_Order_should_be_able_to_remove_all_items()
        {
            var product1 = new Product("part1", 10m);
            var product2 = new Product("part2", 20m);
            var product3 = new Product("part3", 30m);
            var order = new Order();
            order.AddOrderItem(product1, 1);
            order.AddOrderItem(product2, 1);
            order.AddOrderItem(product3, 1);
            Assert.That(order.OrderItems.Count(), Is.EqualTo(3));
            order.RemoveAllItems();
            Assert.That(order.OrderItems.Count(), Is.EqualTo(0));
        }

If you know where this is going, you’re at least one step ahead of me when I wrote the RemoveAllItems function. It does exactly what it is supposed to do, and this test passes. There’s more than one way to implement the RemoveAllItems function and initially I chose a bad one. The following is the original implementation. Looking back, it’s obvious that it is wrong, but at the time, it must have been “the way” to do it.

    public class Order : IOrder
    {
        private IList<IOrderItem> orderItems;
        // ...

        public Order()
        {
            orderItems = new List<IOrderItem>();
            // ...
        }

        public virtual IEnumerable<IOrderItem> OrderItems
        {
            get { return orderItems; }
        }

        public void RemoveAllItems()
        {
            orderItems = new List<IOrderItem>();
            // ...
        }
    }

I’ve seen the ‘new’ keyword cause me trouble before, it usually gives me hard to test code. This time I fell short by not thinking enough about it. (This also could have been avoided if the code was written while pairing, but that’s another story). Why on Earth should the RemoveAllItems create a new List<T>? It shouldn’t, it should call orderItems.Clear(). The following test looks fairly trivial, but is now in place over some code that reared its ugly head at a really bad time.

        [Test]
        public void An_Orders_Items_should_be_the_same_object_when_emptied()
        {
            var product = new Product("part1", 10m);
            var order = new Order();
            var itemsAtCreation = order.OrderItems;

            order.AddOrderItem(product, 1);

            Assert.That(order.OrderItems.Count(), Is.EqualTo(1));
            Assert.That(order.OrderItems, Is.EqualTo(itemsAtCreation));

            order.RemoveAllItems();
            order.AddOrderItem(product, 1);

            Assert.That(order.OrderItems.Count(), Is.EqualTo(1));
            Assert.That(order.OrderItems, Is.EqualTo(itemsAtCreation));
        }

Doing it the wrong way

With this new test in place here are the results I get:

        public void RemoveAllItems()
        {
            orderItems = new List<IOrderItem>();
            // ...
        }

Expected and actual are both <System.Collections.Generic.List`1[Company.Domain.Cart.ICartItem]> with 1 elements
Values differ at index [0]
Expected: <Company.Domain.Cart.CartItem>
But was:  <Company.Domain.Cart.CartItem>

And now the right way

Now that it’s modified like the following, the test passes.

        public void RemoveAllItems()
        {
            orderItems.Clear();
            // ...
        }
1 passed, 0 failed, 0 skipped, took 1.41 seconds.

Why does it matter that the Object is the same?

Not only should it be the same object, in a sense that one order has one collection of order items for its lifetime, but let us imagine that we want to persist this data (reaching, I know). Let us also imagine that we NHibernate for persistence. Combine these factors and not using “all-delete-orphan” with one-shot-deletes, and you’ve got yourself some fairly unexpected behavior.

Related:

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

About Chris Missal

Oh hey, I'm a Senior Consultant for Headspring in Austin, TX. I've been working in software professionally since 2006 and I really, really love it. I'm mostly in the Microsoft world, but enjoy building computer things of all sorts (to be vague). When I'm not slinging code, I'm probably out and about slinging discs, bowling balls, or good beer with great friends.
This entry was posted in Testing. Bookmark the permalink. Follow any comments here with the RSS feed for this post.
  • http://morten.lyhr.dk Morten Lyhr

    Some toughts…

    1). Should a Order follow double bookkeeping, so insted of remove the orderlines add a new one with a negative amount?

    2). “order.OrderItems” breaks the law of demiter, I think thats why it fails not because of the “new List<>“. You are exposing state, so when you alter the state, som clients might get surprised ;-)

    jus my 2 cents…

  • http://www.lostechies.com/members/chrismissal/default.aspx Chris Missal

    @Morten

    1) We could have implemented your first suggestion, I don’t know what we would have gained from a “negative addition” function. I could see this being very useful in some domains, it just didn’t fit in ours.

    2) From the Law of Demeter standpoint, do you mean because OrderItems is not a wrapper class? I think it’s okay in that Order isn’t relying on, and reaching into another objects components. Maybe, I’m missing something else?

  • http://www.patmaddox.com Pat Maddox

    You could ask the order for the number of items it has, rather than asking it for the collection and then checking the size. Currently you’re breaking encapsulation by handing out the collection of order items. One point of OO is that you *should* be able to switch between those two implementations without breaking clients. So it’s not that the first implementation is wrong, but rather that the design leaves something to be desired.

    I wrote a blog post discussing it in more depth, as well as the Aggregate pattern from Domain Driven Design: http://www.patmaddox.com/blog/aggregate_pattern_and_encapsulation