Shopping cart code kata

Question

I have recently started code kata and learning for myself, as I would like to learn test-first development. I have started doing a shopping cart as my code kata. I have included a test as well. I would like to know what are the ways I could improve my below code. I know I need to use dependency injection next step. If you find anything I can improve in my code, please let me know.

using System.Collections;
using System.Collections.Generic;
using System.Linq;
using NUnit.Framework;

namespace Domain.Tests.Shopping_Cart_Tests
{
    [TestFixture]
    public class ShoppingCartTest
    {
        private IShoppingCart _shoppingCart;

        [SetUp]
        public void SetUp()
        {
            _shoppingCart = new ShoppingCart();
        }

        [Test]
        public void ShouldBeAbleToAddToCart()
        {
            var item1 = new CartItem{ProductId="P1001",Name = "Logitech Mouse", Qty = 1, Price = 5.00};

            _shoppingCart.AddToCart(item1);

            Assert.AreEqual(_shoppingCart.GetCartItems().Count, 1);
        }

        [Test]
        public void ShouldBeAbleToAddToCartMultipleItems()
        {
            var item1 = new CartItem { ProductId = "P1001", Name = "Logitech Mouse", Qty = 1, Price = 5.00 };
            var item2 = new CartItem { ProductId = "P1002", Name = "Logitech Keyboard", Qty = 1, Price = 9.00 };

            _shoppingCart.AddToCart(item1);
            _shoppingCart.AddToCart(item2);

            Assert.AreEqual(_shoppingCart.GetCartItems().Count, 2);
        }

        [Test]
        public void ShouldBeAbleToIncrementWhenAddTheSameItem()
        {
            var item1 = new CartItem { ProductId = "P1001", Name = "Logitech Mouse", Qty = 1, Price = 5.00 };
            var item2 = new CartItem { ProductId = "P1001", Name = "Logitech Mouse", Qty = 2, Price = 5.00 };
            var item3 = new CartItem { ProductId = "P1001", Name = "Logitech Mouse", Qty = 3, Price = 5.00 };


            _shoppingCart.CheckItemExistThenAddToCart(item1);
            Assert.AreEqual(_shoppingCart.GetCartItems().Count, 1);
            _shoppingCart.CheckItemExistThenAddToCart(item2);
            Assert.AreEqual(_shoppingCart.GetCartItems().Count, 1);
            _shoppingCart.CheckItemExistThenAddToCart(item3);
            Assert.AreEqual(_shoppingCart.GetCartItems().Count,1);
        }
    }

    public interface IShoppingCart
    {
        void AddToCart(CartItem item);
        List<CartItem> GetCartItems();
        void CheckItemExistThenAddToCart(CartItem item);
    }

    public class ShoppingCart : IShoppingCart, IEnumerable
    {
        private static List<CartItem> CartItems = null;

        public ShoppingCart()
        {
            CartItems = new List<CartItem>();
        }

        public void AddToCart(CartItem item)
        {
            CartItems.Add(item);
        }

        public void CheckItemExistThenAddToCart(CartItem item)
        {
            var cartItem = CartItems.FirstOrDefault(ci => ci.ProductId == item.ProductId);
            if (cartItem == null)
                AddToCart(item);
            else
                cartItem.Qty += item.Qty;
        }

        public List<CartItem> GetCartItems()
        {
            return CartItems;
        }

        public IEnumerator GetEnumerator()
        {
            return (CartItems as IEnumerable).GetEnumerator();
        }
    }

    public class CartItem
    {
        public string Name { get; set; }

        public double Price { get; set; }

        public int Qty { get; set; }

        public string ProductId { get; set; }
    }
}

Answer 1

1. public double Price { get; set; } clearly should be of type Decimal. That is what Decimal for.
2. private static List<CartItem> CartItems = null; can be safely refactored to Dictionary<int, CartItem> with ProductId used as keys. Then CheckItemExistThenAddToCart can be refactored using Dictionary.ContainsKey method, and GetCartItems can return dictionary itself, CartItems.Values or CartItems.Values.ToList(). I think the latter option is the best, since it will copy the collection, which will restrict access to CartItems property. As of now, someone might modify it in outer code, which will result it modifying ShoppingCart state. Returning a copy will prevent that.
3. return (CartItems as IEnumerable).GetEnumerator(); cast can be removed.
4. I'm not sure i like the idea of having two methods to add items to the cart, which have different logic. It's pretty error-prone. Is it really necessary? I think you should probably stick to one.
5. I think your shopping cart interface misses Remove method.

Your unit tests look fine to me (except you should move them to different file), tho i'm not too experienced with them.

Answer 2

Comments as method names

public void CheckItemExistThenAddToCart(CartItem item)

The following is sufficient:

public void AddItem( CartItem item)

... but a XML comment would be excellent:

///<param name="item">
/// Does nothing if null. Does not throw an exception
///</param>

Commenting code

---

And this could also be fine:

public void Add( CartItem item)

... because the parameter tells us what's being added.

---

Multiple Add methods

Makes no sense. Why would a method arguement be validated sometimes and other times not? How would a maintenance programmer be expected to know when or why to call one or the other?

If indeed multiple "add" methods are needed then overloading is what you want. Overloading is a fabulous way of saying "hey, you can do the same thing, but differently" Not overloading obscures the fact that the methods all do the same thing. Then, overloading highlights what's important - that with different object types we get the same result.

---

Superfluous Tests

I argue that the two "addToCart" tests is one too many. There's nothing specical about adding one and then several others; unless there are special requirements like duplicate items not allowed. Or something special about adding objects with particular different properties.

Instead I rather see how passing null works. This is clearly a different/special case.

---

Use messages in tests

I strongly urge in the most emphatic way that you add a message parameter to every test. When you are reading oodles of useless generic test output that says, like, "expected to be equal but was not" you'll understand. Also the test method signature becomes more self documenting and is a huge help for anyone reading the code. Again, imagine dozens and hundreds of tests that you need to make sense of.

---

Code file organization

Putting the code under a "testing" namespace is misleading, the seed for bad solution organization, and just lazy.

The convention I've learned is that test code is not only in separate files but also in its own project(s), generally with file and project naming that "pairs" it with the particular target code.

As your applications grow and you implement continuous integration you'll be glad you took the time.

---

CartItem

Yeah, I know that writing short, so called Data Transfer Objects (DTO) is a thing. But inappropriate here. If you want to ".. use dependency injection next step", start here. Use a constructor to inject the values. Thus YOU, not the client code, guarantee complete, valid, immuteable objects.

Simple immuteability is its own virtue. The very worst of our code base is due in very large part to changing object properties willy-nilly.

You really, really, really need to develop a defensive mindset, generally. Write code that can't be FUBARed. By hiding state and exposing appropriate functionality you force the coder to do the right thing. And at the same time communicate design, the domain, and behavior. Think "Application Public Interface."

---

Single Responsibility - ShoppingCart

Should calcualte it's own total. Client code should only ask for the total, not calculate it. Taxes calculation may or may not be in here. Depends on how the design and requirements evolve.

Don't expose the cart items list to clients; at least I don't think you need too given the code so far.

Consider ToString() override for a nicely formatted "reciept".

---

Single Responsibility - An items collection

This is a really excellent exercise for understanding SRP.

Make a ItemsCollection class. Put the enumerator in here. Have simple properties like "total price", "total quantity" and any others appropriate for lists of things.

Override ToString() for a nice listing. Refactor ShoppingCart.ToString to use that. And generally fine tune the item list and shopping cart classes for SRP-ness.

ShoppingCart will still have a total method; which will call the collection total method. This is one example of how a shopping cart client will talk to the shopping cart and be ignorant of shopping cart implementation. It is also an example of "Maximize cohesion and minimize coupling" (I love that phrase!).

As for general implementation I prefer to "have a" generic List (or other appripriate NET-supplied collection class) instead of inheriting from List or implementing ICollection, etc. Why? Because it hides all the .NET-supplied pubilc properties and methods and I expose precisely and only the functionality I want. It goes to design intent, SRP, defensive coding, user requirements, domain specific language, etc.

---

Single Responsibility - Custom collections

For me has become a no-brainer for good SRP application and enhanced functionality. The .NET framework collections take advantage of "equals", "IComparable", and "IEquateable" (more?). All of a sudden "Find", "Sort", "Contains" collection methods work like magic (from the client code perspective).