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? 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.