3
\$\begingroup\$

I have done the supermarket pricing kata in TDD style and I would appreciate it if someone could review it for me.

Kata:

"...checkout that calculates the total price of a number of items. In a normal supermarket, things are identified using Stock Keeping Units, or SKUs. In our store, we’ll use individual letters of the alphabet (A, B, C, and so on). Our goods are priced individually. In addition, some items are multipriced: buy n of them, and they’ll cost you y cents. For example, item ‘A’ might cost 50 cents individually, but this week we have a special offer: buy three ‘A’s and they’ll cost you $1.30. In fact this week’s prices are:"

| Item Name  | Price       | Special Price |
|:-----------|------------:|:------------: |
| A          |      50     |    3 for 130  |
| B          |      30     |    2 for 45   |

"Our checkout accepts items in any order, so that if we scan a B, an A, and another B, we’ll recognize the two B’s and price them at 45 (for a total price so far of 95). Because the pricing changes frequently, we need to be able to pass in a set of pricing rules each time we start handling a checkout transaction. "

Tests:

public class CheckoutTests
    {
        private Checkout checkout;

        private readonly Item itemA = new Item
                                      {
                                          Name = "A",
                                          Price = 50
                                      };

        private readonly Item itemB = new Item
                                      {
                                          Name = "B",
                                          Price = 30
                                      };

        private readonly PricingRule ItemARule = new PricingRule
                                                 {
                                                     ItemName = "A",
                                                     ItemCount = 3,
                                                     Total = 130
                                                 };

        private readonly PricingRule ItemBRule = new PricingRule
                                                 {
                                                     ItemName = "B",
                                                     ItemCount = 2,
                                                     Total = 45
                                                 };


        [SetUp]
        public void SetUp()
        {
            checkout = new Checkout();
        }

        private void AddItems(int count,
                              Item item)
        {
            for (int i = 0;
                 i < count;
                 i++)
            {
                checkout.AddItem(item);
            }
        }

        [Test]
        public void TestCheckoutZeroItemReturnsZero()
        {
            int result = checkout.CalculateTotal();

            Assert.AreEqual(0,
                            result);
        }

        [Test]
        public void TestCheckoutOneItemsPriceAsTotal()
        {
            AddItems(1,
                     itemA);

            int result = checkout.CalculateTotal();
            Assert.AreEqual(50,
                            result);
        }

        [Test]
        [ExpectedException(typeof (ArgumentException))]
        public void TestAddingPricingRuleWithoutNameThrowsException()
        {
            checkout.AddPricingRule(new PricingRule
                                    {
                                        ItemCount = 2,
                                        Total = 45
                                    });
        }

        [Test]
        public void TestCheckoutWithSpecialPriceCriteriaMetReturnsSpecialPrice()
        {
            checkout.AddPricingRule(ItemARule);

            AddItems(3,
                     itemA);

            int result = checkout.CalculateTotal();

            Assert.AreEqual(130,
                            result);
        }

        [Test]
        public void TestCheckoutWithSpecialPriceCriteriaNotMetReturnsTotal()
        {
            checkout.AddPricingRule(ItemARule);

            AddItems(2,
                     itemA);

            int result = checkout.CalculateTotal();

            Assert.AreEqual(100,
                            result);
        }

        [Test]
        public void TestCheckoutWithSpecialPriceCriteriaMetAndExtraItemsReturnsSpecialPricePlusExtraItemsTotal()
        {
            checkout.AddPricingRule(ItemARule);

            AddItems(4,
                     itemA);

            int result = checkout.CalculateTotal();

            Assert.AreEqual(180,
                            result);
        }

        [Test]
        public void TestCheckoutWithMultipleGroupsOfItemsWithSpecialPriceCriteriaMet()
        {
            checkout.AddPricingRule(ItemARule);
            checkout.AddPricingRule(ItemBRule);

            AddItems(4,
                     itemA);
            AddItems(3,
                     itemB);


            int result = checkout.CalculateTotal();

            Assert.AreEqual(255,
                            result);
        }
    }

Implementation:

public class Checkout
    {
        private readonly List<Item> items = new List<Item>();
        private readonly List<PricingRule> pricingRules = new List<PricingRule>();


        public int CalculateTotal()
        {
            int total = 0;
            var itemGroups = items.GroupBy(g => g.Name);

            foreach (var itemGroup in itemGroups)
            {
                var ruleForGroup = pricingRules.FirstOrDefault(r => r.ItemName == itemGroup.Key);
                if (ruleForGroup != null)
                {
                    var groupCount = itemGroup.Count();

                    var extra = groupCount - ruleForGroup.ItemCount;
                    if (extra < 0)
                    {
                        total += itemGroup.Sum(g => g.Price);
                    }
                    else
                    {
                        total += ruleForGroup.Total;
                        total += extra*itemGroup.First()
                                                .Price;
                    }
                }
                else
                {
                    total += itemGroup.Sum(x => x.Price);
                }
            }
            return total;
        }

        public void AddItem(Item item)
        {
            items.Add(item);
        }

        public void AddPricingRule(PricingRule rule)
        {
            if (rule == null)
            {
                throw new ArgumentNullException();
            }

            if (string.IsNullOrWhiteSpace(rule.ItemName))
            {
                throw new ArgumentException();
            }

            pricingRules.Add(rule);
        }
    }
\$\endgroup\$
  • \$\begingroup\$ Did you continuously apply refactoring when writing the above? \$\endgroup\$ – rjnilsson Jun 16 '15 at 12:35
  • \$\begingroup\$ @Cwan only to the tests, not to the implementation. \$\endgroup\$ – xerxes Jun 16 '15 at 12:37
  • 3
    \$\begingroup\$ It's not really TDD unless you refactor mercilessly during each iteration of adding functionality. You have many opportunities to improve your implementation above. \$\endgroup\$ – rjnilsson Jun 16 '15 at 13:48
  • \$\begingroup\$ I started to write another review, but decided it would be more fun to do this myself. You might be interested in how I did it with the strategy pattern. \$\endgroup\$ – RubberDuck Jun 20 '15 at 21:54
10
\$\begingroup\$

ArgumentExceptions

public void AddPricingRule(PricingRule rule)
{
    if (rule == null)
    {
        throw new ArgumentNullException();
    }

You're null checking arguments! Awesome! You even remembered the test for it! Even better!

But what about this?

public void AddItem(Item item)
{
    items.Add(item);
}

It's public, so you should be checking it's argument too.

Since we're talking about this code block though, let's talk about how you're throwing exceptions.

if (rule == null)
{
    throw new ArgumentNullException();
}

if (string.IsNullOrWhiteSpace(rule.ItemName))
{
    throw new ArgumentException();
}

It's not very helpful to throw argument exceptions without telling the dev working on the client code what they did wrong. The first one... okay. There's only one arg. Okay, but the latter one? How would I ever know what I did wrong without digging up the implementation? What if I couldn't because I had the compiled version of the library? Let's add some extra info here.

if (rule == null)
{
    throw new ArgumentNullException("rule");
}

if (string.IsNullOrWhiteSpace(rule.ItemName))
{
    throw new ArgumentException("rule.ItemName cannot be null or whitespace.");
}

But that's not very nice is it? If we rename that parameter, our message is going to lie. We can fix that if we're using C# 6.0 using nameof and string interpolation.

if (rule == null)
{
    throw new ArgumentNullException(nameof(rule));
}

if (string.IsNullOrWhiteSpace(rule.ItemName))
{
    throw new ArgumentException($"{nameof(rule)}.{nameof(rule.ItemName)} cannot be null or whitespace.");
}
\$\endgroup\$
5
\$\begingroup\$

Next Kata Cycle: Object Oriented

Taking an OO approach will make the code more Openy-Closey because good classes are focused and then tend to have "one reason to change." This good application of Single Responsibility Principle gives the classes a quality such that the Open-Close principle is a natural byproduct.

In otherwords O/C principle is not a guide for construction as much as it is an indication of well formed classes.

SPECIAL NOTE: It will be a long time before I get around to actually calculating the grocery total cost. There is a lot of OO structure to get into place.

SPECIAL NOTE 2: These are in no particular order and are not complete. This is a sketch to get across the idea if thinking object oriented.


items. In a normal supermarket, things are identified using Stock Keeping Units, or SKUs. In our store, we’ll use individual letters of the alphabet (A, B, C, and so on).

public class GroceryItem
{
    public Char SKU { get; protected set; }  // yeah, I know, limit of 26. But, hey refactoring is what its about.
    public string Name { get; protected set; }
    public double Price { get; protected set; }
}

Our checkout

public class Checkout{}

accepts items in any order

public class CheckOut {
    public void PurchaseItem (GroceryItem newItem) {}
}

this week we have a special offer:

public class SpecialOffer {}

... set of pricing rules ...

public class SpecialOfferCollection : List<SpecialOffer> {}

Getting to OO

things are identified using Stock Keeping Units, or SKUs.

public class GroceryItem{
    public override bool Equals (Object other) {
        if (obj == null || GetType() != obj.GetType())
            return false;

        return SKU == ((GroceryItem)obj).SKU;
    }
}

"Honey, I'm going to the store for a collection of GroceryItems." Really, I say this all the time.

public class GroceryItemCollection : List<GroceryItem>
{
    public GroceryItemCollection()  {}

    public void AddItem(GroceryItem newItem)
    {
        if (newItem == null)
            return;

        this.Add(newItem);
    }
}

Hmmm... do I have any coupons?

public class SpecialOffer {
    public GroceryItem theItem { get; protected set; }

    public SpecialOffer (GroceryItem newDeal) {
        if( newDeal == null )
            return;

        theItem = newDeal;
    }
}

Oh! I found a bunch of coupons!

public class SpecialOfferCollection : List<SpecialOffer> {
    public void AddOffer( SpecialOffer goodDeal ) {
        if( goodDeal == null ) 
             return;

        this.Add(goodDeal);
    }
}

Thinking about checkout process.

Changes CheckOut class name, and some miscellaneous

public class Cashier {
    protected GroceryItemCollection Groceries { get; protected set; }
    protected SpecialOfferCollection Coupons { get; protected set; }

    public Cashier ( GroceryItemCollection groceries, SpecialOfferCollection coupons ) {
        if (groceries == null) throw new ArgumentNullException();
        // don't care if there are no coupons
        Coupons = coupons;
        Groceries = groceries;
    }
    public decimal CheckOut( GroceryItemCollection customerGroceries ) {
        // STOP. Let's think about calculating total in an OO way.
}

The GroceryItemCollection is the natural place to calculate a total.

public class GroceryItemCollection {
    protected decimal Total { get; set; }

    public decimal Total() {
        Total = 0;

        foreach (var item in this) 
            Total += item.Price;

        return Total;
}

buy three ‘A’s and they’ll cost you $1.30.

The problem with the OP's code is that CheckOut() is trying be-and-do-everything-at-once. THERE ARE 2 THINGS HERE. Groups of items, and then the "Pricing Rule". OO programming is about identifying the parts in our puzzle and then putting them in the appropriate places.

public Dictionary<Char, int> GroupedGroceries; 
   // Char is the Item.SKU and int is a count.

Where do we put it? As the Cashier scans each item, then we can group them. That seems OO-good.

public class Cashier {
    protected Dictionary<Char,int> CheckedOut;

    public decimal CheckOut() {
        CheckedOut = new Dictionary<Char, int>();

        foreach (var item in this.Groceries) {
            if( ! CheckedOut.ContainsKey(item.SKU )
                CheckedOut.Add(item.SKU, 1;
            else
                CheckedOut[item.SKU]++;
        }

// STOP. Don't spew out price calc code here. We need to deal with "we have 3-fer Coupon and we bought 5 of them". But that too will be encapsulated... somewhere; in Openy-Closey think we might consider extending GroceryItemCollection.

// I can imagine it's use thus:

        if(SpecialOfferCollection.IsEmpty)
            Total = Groceries.Total();
        else
            Total = Groceries.TotalWithCoupons();
    }
}

edit

Refactoring Ideas

GroceryItemCollection internally groups items. This will allow the customer's receipt to have a single line for multiple items. Further, it will facilitate applying coupons.

GroceryItemCollection internally applies coupons. The idea is to just say Total(), period. This removes and if/else or other control logic from the Cashier. Better encapsulation and more reusable I think.

Have a CouponApplicator class to encapsulate potentially complex coupon logic. Used by GroceryItemCollection of course.

end edit

\$\endgroup\$
  • \$\begingroup\$ I really impressed by your answer. But I have a question , public class SpecialOfferCollection : List<SpecialOffer> here why you chose inheritance over composition? \$\endgroup\$ – Rahul Raut Jul 29 at 3:49
  • \$\begingroup\$ No good reason. I should have changed that years ago. My preference is composition because I don't want to expose all List public members, use domain relevant method names, and limit the thing to "groceries checkout special offer requirements." \$\endgroup\$ – radarbob Jul 29 at 8:01
4
\$\begingroup\$

I'm not very fond of seeing that you focused on a specific type of rule for adding up the price of your products. Part of the requirements was to be able to add in a set of rules for each time you checkout since their rules change frequently. What if they added a "50% off sale TODAY ONLY!!!" or "When you buy 10 items you get 10% back!" Those type of rules would be very hard to add in without changing your code kinda heavily. One principle to keep in mind when writing your production code (based off of your tests) is the Open Closed Principle. Open for extension, closed for change. Your code would have to change to take on a different discount system. To quote from Uncle Bob (which he may have been quoting someone else, but its still good advice) is that as your test code becomes more specific your production code should get more generic. Judging by your code I would not say that it got more generic, but stayed about the same. So how can a person do this? My suggestion would be a more generic approach. make a interface for your rules, and make the default one be to just sum up the price of all your items. The others that you'd come up with would focus more on the discount.

I ran the code metrics on your code, and CalculateTotals has a 12 for Complexity. Like the comment above stated you have tests in place, refactor. spread out that complexity. Little bits of encapsulation do wonders for being able to read this. For instance, just by pulling out 1 thing the code becomes to me much easier to understand

public int CalculateTotal()
{
    int total = 0;
    var itemGroups = items.GroupBy(g => g.Name);

    foreach (var itemGroup in itemGroups)
    {
        total += PriceTotalForEachGroup(itemGroup);
    }
    return total;
}

private int PriceTotalForEachGroup(IGrouping<string, Item> itemGroup)
{
    int total = 0;
    var ruleForGroup = pricingRules.FirstOrDefault(r => r.ItemName == itemGroup.Key);
    if (ruleForGroup != null)
    {
        var extra = itemGroup.Count() - ruleForGroup.ItemCount;
        if (extra < 0)
        {
            total += itemGroup.Sum(g => g.Price);
        }
        else
        {
            total += ruleForGroup.Total;
            total += extra * itemGroup.First()
                                    .Price;
        }
    }
    else
    {
        total += itemGroup.Sum(x => x.Price);
    }

    return total;
}

By pulling that out I start to see what the code is wanting to do. I thought to myself, what is this total doing in the else statement? I commented it out and ran the tests to see what failed and it is just the case where a discount rule is not found for a grouping, or to put it differently it should be applied when a discount isn't applied. Since this is a checkout scenario anything not at discount is at full price, and since I am checking out I should have the price added always anyway! Which brings another point of change to light. What if the customer wants to see all items printed out at full price, then show a list of discounts in the receipt? (I know at least 2 companies that do that)

That's my two cents. Open Close Principle. Try that approach next time you do this Kata.

\$\endgroup\$
  • \$\begingroup\$ What about YAGNI? \$\endgroup\$ – Roland Illig Oct 17 '17 at 6:57
  • \$\begingroup\$ @RolandIllig yes YAGNI does play a factor in this and the Open Close principle tries to keep YAGNI in place by making it easy to add in what you now do need. I didn't mention it since I only wanted to mention OCP \$\endgroup\$ – Robert Snyder Oct 17 '17 at 12:43
2
\$\begingroup\$

I know this is a little old, but the Checkout kata is one of my favorites and I wanted to add my 2 cents.

Looking at the original submission and a couple previous answers, I think they are all a little more complicated than they need to be. This is mostly because they have 2 types of pricing rules: one for prices and one for discounts. Besides not exactly conforming to the original kata instructions, which show injecting just 1 set of RULES, it's easy enough to do with just 1 set of rules.

My rules implement the following interface.

public interface IShoppingCartRule
    {
        char Sku {get;}
        string Description { get; }
        decimal Value { get; }
        bool Evaluate(char sku, IEnumerable<char> skus);
    }

The key is that there can be multiple rules for the same SKU. Therefore there is a PriceRule('A', "Apples", 50), and a NForRule('A', "Apples 3 for 130", -20, 3). All the price/discount logic goes into Evaluate and can be as simple or complicated as needed. Passing in the list of scanned SKUs allows for rules to apply based on the number of items.

   public class NForRule : IShoppingCartRule
    {
        private char sku;
        public Char Sku { get { return sku; } }

        private string description;
        public string Description { get { return description; } }

        private decimal value;
        public decimal Value { get { return value; } }

        private int count;
        public int Count { get { return count; } }

        public bool Evaluate(char sku, IEnumerable<char> skus)
        {
            return skus.Count(s => s == sku) % count == 0;
        }

        public NForRule(char sku, string description, decimal value, int count)
        {
            this.sku = sku;
            this.description = description;
            this.value = value;
            this.count = count;
        }
    }

As items are scanned, I get a collection of RuleResult that I add to a private property. Total is just a Sum of the Value property of items in the results.

public class RuleResult
    {
        private char sku;
        public char Sku { get { return sku; } }

        private string description;
        public string Description { get { return description; } }

        private decimal value;
        public decimal Value { get { return value; } }

        public RuleResult(char sku, string description, decimal value)
        {
            this.sku = sku;
            this.description = description;
            this.value = value;
        }
    }

This makes the ShoppingCart class very simple:

   public class ShoppingCart
    {
        private List<IShoppingCartRule> rules;

        private List<char> skus;
        private List<RuleResult> results;

        public decimal Total
        {
            get
            {
                return results.Sum(r => r.Value);
            }
        }

        public ShoppingCart(List<IShoppingCartRule> rules)
        {
            this.rules = rules;

            this.skus = new List<char>();
            results = new List<RuleResult>();
        }

        public IEnumerable<RuleResult> Scan(char sku)
        {
            skus.Add(sku);
            var result = rules.Where(r => r.Sku == sku).Where(r => r.Evaluate(sku, skus)).Select(r => new RuleResult(r.Sku, r.Description, r.Value));
            results.AddRange(result);
            return result;
        }
    }

The only complicated part is the linq query that selects rules where the skus match and then Evaluate to true, and then creates a RuleResult where applicable.

ShoppingCart should not have to change for changes in pricing. You can create new pricing rules as you see fit. I could get rid of the results collection and calculate Total on the fly, but I like returning the RuleResults and having the collection to create a receipt.

I have also extended it by adding IsTaxable properties to the rules and results and adding a Tax property to ShoppingCart that simply sums the results where IsTaxable == true.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.