HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Shopping cart code kata

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
codekatacartshopping

Problem

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

Solution


  • public double Price { get; set; } clearly should be of type Decimal. That is what Decimal for.



  • private static List CartItems = null; can be safely refactored to Dictionary 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.



  • return (CartItems as IEnumerable).GetEnumerator(); cast can be removed.



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



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

Context

StackExchange Code Review Q#30600, answer score: 2

Revisions (0)

No revisions yet.