7
\$\begingroup\$

I saw this question and thought it looked like a great opportunity to try my hand at the Strategy Pattern. I've never used it before, but I think I did pretty well. Did I?

The code below can also be found on github. I'm using C# 6.0 for this, so if there are any opportunities to use new features that I missed, I'd like to know.

The Challenge

"...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   |
| C          |      20     |               |
| D          |      15     |               |

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

Back To The Checkout

The Approach

  • I created a Sku struct for type safety and to allow myself to implement more realistic Skus down the road if I choose to.
  • The Cashier is instantiated with a a List of Pricing Strategies.
  • The Cashier is then responsible for checking out a list of Skus and returning the total price.
  • Each strategy implements a IPricingStrategy interface.
  • Two different base classes fell out of my TDD exercise. A "Regular" and a "Sale" strategy. Strategies for specific products inherit from one of these two base classes.

My Concerns

  1. Is my implementation of the strategy pattern correct?
  2. Right now, I would have to implement a new class for each sku. This would be impractical (to say the least) with hundreds of thousands of skus. Was the strategy pattern a bad choice? How could the code be made more resilient and re-usable over many skus that use the same strategy?
  3. I choose to use a custom Sku struct as a datatype for type safety, but my implementation doesn't feel like it did that well. It allows any char to be implicitly cast to a Sku. It feels like I defeated the purpose. Did I? If I did, how could I regain the type safety I was aiming for?
  4. Did I properly implement my Sku struct in regards to overriding Equals(), GetHashCode(), and overloading the operators?
  5. I wrote my tests at a very high level, to test the results from Cashier.Checkout(). Should I go down a level and test the Strategies too, or instead of?

The Code

Sku.cs

namespace SuperMarketPricing
{
    public struct Sku
    {
        private char _value;

        public Sku(char value)
        {
            _value = value;
        }

        public Sku(Sku sku)
        {
            _value = sku._value;
        }

        public static implicit operator Sku(char v)
        {
            return new Sku(v);
        }

        public static explicit operator char (Sku v)
        {
            return v._value;
        }

        public static bool operator ==(Sku sku1, Sku sku2)
        {
            return sku1.Equals(sku2);
        }

        public static bool operator !=(Sku sku1, Sku sku2)
        {
            return !sku1.Equals(sku2);
        }

        public override bool Equals(object obj)
        {
            if (obj == null) { return false; }

            Sku sku;
            try
            {
                sku = (Sku)obj;
            }
            catch
            {
                return false;
            }

            return _value == (char)sku;
        }

        public override int GetHashCode()
        {
            return _value.GetHashCode();
        }
    }
}

Cashier.cs

using System.Collections.Generic;
using System.Linq;

namespace SuperMarketPricing
{
    public class Cashier
    {
        private List<IPricingStrategy> _pricingStrategies;

        public Cashier(List<IPricingStrategy> pricingStrategies)
        {
            _pricingStrategies = pricingStrategies;
        }

        public double Checkout(IList<Sku> products)
        {
            double result = 0;
            foreach(var strat in _pricingStrategies)
            {
                var prods = products.Where(p => p == strat.Sku);
                result = result + strat.GetPrice(prods.Count());
            }

            return result;
        }
    }
}

IPricingStrategy.cs

namespace SuperMarketPricing
{
    public interface IPricingStrategy
    {
        Sku Sku { get; } 
        double GetPrice(int count);
    }
}

SaleStrategies.cs

namespace SuperMarketPricing
{
    public abstract class SaleStrategy : IPricingStrategy
    {
        public abstract Sku Sku { get; }
        protected abstract double PricePerOne { get; }
        protected abstract double PricePerX { get; }
        protected abstract int X { get; }

        public double GetPrice(int count)
        {
            if (count == 0)
            {
                return 0;
            }

            double result = 0;

            while (count >= X)
            {
                result = result + PricePerX;
                count = count - X;
            }

            return result + (PricePerOne * count);   
        }
    }

    public class PricingStategyA : SaleStrategy
    {
        public override Sku Sku { get; } = 'A';
        protected override double PricePerOne { get; } = 50;
        protected override double PricePerX { get; } = 130;
        protected override int X { get; } = 3;
    }

    public class PricingStrategyB : SaleStrategy
    {
        public override Sku Sku { get; } = 'B';
        protected override double PricePerOne { get; } = 30;
        protected override double PricePerX { get; } = 45;
        protected override int X { get; } = 2;
    }
}

RegularStrategies.cs

namespace SuperMarketPricing
{
    public abstract class RegularStrategy : IPricingStrategy
    {
        public abstract Sku Sku { get; }
        protected abstract double Price { get; }

        public double GetPrice(int count)
        {
            return Price * count;
        }
    }

    public class PricingStrategyC : RegularStrategy
    {
        public override Sku Sku { get; } = 'C';
        protected override double Price { get; } = 20;
    }

    public class PricingStrategyD : RegularStrategy
    {
        public override Sku Sku { get; } = 'D';
        protected override double Price { get; } = 15;
    }
}

The Tests

What's a TDD kata without the tests?

SkuTests.cs

using System;
using Microsoft.VisualStudio.TestTools.UnitTesting;

namespace SuperMarketPricing.Tests
{
    [TestClass]
    public class SkuTests
    {
        [TestMethod]
        public void Sku_ImplicitCastFromCharToSku()
        {
            var sku = new Sku('A');
            sku = 'B';

            Assert.AreEqual('B', sku);
        }

        [TestMethod]
        public void Sku_ImplicitCastFromSkuToSku()
        {
            var original = new Sku('A');
            var expected = new Sku('B');

            original = expected;

            Assert.AreEqual(expected, original);
        }

        [TestMethod]
        public void Sku_ExplicitCastFromSkuToChar()
        {
            var sku = new Sku('A');

            Assert.AreEqual('A', (char)sku);
        }

        [TestMethod]
        public void Sku_ExplicitCastFromCharToSku()
        {
            var sku = new Sku('A');

            Assert.AreEqual(sku, (Sku)'A');
        }

        [TestMethod]
        public void Sku_CharAndSkuHaveSameHashCode()
        {
            var sku = new Sku('A');
            var chr = 'A';

            Assert.AreEqual(chr.GetHashCode(), sku.GetHashCode());
        }

        [TestMethod]
        public void Sku_SkuAndSkuHaveSameHashCode()
        {
            var sku1 = new Sku('A');
            var sku2 = new Sku('A');

            Assert.AreEqual(sku1.GetHashCode(), sku2.GetHashCode());
        }

        [TestMethod]
        public void Sku_CanCreateNewSkuFromExisting()
        {
            var sku1 = new Sku('A');
            var sku2 = new Sku(sku1);

            Assert.AreEqual(sku1, sku2);
        }

        [TestMethod]
        public void Sku_CanUseEqualsOperator()
        {
            Sku sku1 = 'A';
            Sku sku2 = 'A';

            Assert.IsTrue(sku1 == sku2);
        }

        [TestMethod]
        public void Sku_TwoDifferentSkusAreUnEqual()
        {
            Sku sku1 = 'A';
            Sku sku2 = 'B';

            Assert.AreNotEqual(sku1, sku2);
        }
    }
}

CashierTests.cs

using System.Collections.Generic;
using Microsoft.VisualStudio.TestTools.UnitTesting;

namespace SuperMarketPricing.Tests
{
    [TestClass]
    public class CashierTests
    { 
        [TestMethod]
        public void Cashier_WhenNoProducts_PriceIsZero()
        {
            var cashier = new Cashier(GetPricingStrategies());
            var products = new List<Sku>();

            var price = cashier.Checkout(products);

            Assert.AreEqual(0, price);
        }

        [TestMethod]
        public void Cashier_OneA_Is50()
        {
            var cashier = new Cashier(GetPricingStrategies());
            var products = new List<Sku>() { 'A' };

            var price = cashier.Checkout(products);

            Assert.AreEqual(50, price);
        }

        [TestMethod]
        public void Cashier_TwoA_Is100()
        {
            var cashier = new Cashier(GetPricingStrategies());
            var products = new List<Sku>() { 'A', 'A' };

            var price = cashier.Checkout(products);

            Assert.AreEqual(100, price);
        }

        [TestMethod]
        public void Cashier_ThreeA_Is130()
        {
            var cashier = new Cashier(GetPricingStrategies());
            var products = new List<Sku>() { 'A', 'A', 'A' };

            var price = cashier.Checkout(products);

            Assert.AreEqual(130, price);
        }

        [TestMethod]
        public void Cashier_FourA_Is180()
        {
            var cashier = new Cashier(GetPricingStrategies());
            var products = new List<Sku>() { 'A', 'A', 'A', 'A' };

            var price = cashier.Checkout(products);

            Assert.AreEqual(180, price);
        }

        [TestMethod]
        public void Cashier_SixA_Is260()
        {
            var cashier = new Cashier(GetPricingStrategies());
            var products = new List<Sku>() { 'A', 'A', 'A', 'A', 'A', 'A' };

            var price = cashier.Checkout(products);

            Assert.AreEqual(260, price);
        }

        [TestMethod]
        public void Cashier_OneB_Is30()
        {
            var cashier = new Cashier(GetPricingStrategies());
            var products = new List<Sku>() { 'B' };

            var price = cashier.Checkout(products);

            Assert.AreEqual(30, price);
        }

        [TestMethod]
        public void Cashier_TwoBIs45()
        {
            var cashier = new Cashier(GetPricingStrategies());
            var products = new List<Sku>() { 'B', 'B' };

            var price = cashier.Checkout(products);

            Assert.AreEqual(45, price);
        }

        [TestMethod]
        public void Cashier_ThreeBIs75()
        {
            var cashier = new Cashier(GetPricingStrategies());
            var products = new List<Sku>() { 'B', 'B', 'B' };

            var price = cashier.Checkout(products);

            Assert.AreEqual(75, price);
        }

        public void Cashier_FourBIs90()
        {
            var cashier = new Cashier(GetPricingStrategies());
            var products = new List<Sku>() { 'B', 'B', 'B', 'B' };

            var price = cashier.Checkout(products);

            Assert.AreEqual(90, price);
        }

        [TestMethod]
        public void Cashier_OneAOneB_Is80()
        {
            var cashier = new Cashier(GetPricingStrategies());
            var products = new List<Sku>() { 'A', 'B' };

            var price = cashier.Checkout(products);

            Assert.AreEqual(80, price);
        }

        [TestMethod]
        public void Cashier_OneATwoB_OutOfOrder_Is95()
        {
            var cashier = new Cashier(GetPricingStrategies());
            var products = new List<Sku>() { 'B', 'A', 'B' };

            var price = cashier.Checkout(products);

            Assert.AreEqual(95, price);
        }

        [TestMethod]
        public void Cashier_OneC_Is20()
        {
            var cashier = new Cashier(GetPricingStrategies());
            var products = new List<Sku>() { 'C' };

            var price = cashier.Checkout(products);

            Assert.AreEqual(20, price);
        }

        [TestMethod]
        public void Cashier_TwoC_Is20()
        {
            var cashier = new Cashier(GetPricingStrategies());
            var products = new List<Sku>() { 'C', 'C' };

            var price = cashier.Checkout(products);

            Assert.AreEqual(40, price);
        }

        [TestMethod]
        public void Cashier_OneD_Is15()
        {
            var cashier = new Cashier(GetPricingStrategies());
            var products = new List<Sku>() { 'D' };

            var price = cashier.Checkout(products);

            Assert.AreEqual(15, price);
        }

        [TestMethod]
        public void Cashier_TwoD_Is30()
        {
            var cashier = new Cashier(GetPricingStrategies());
            var products = new List<Sku>() { 'D', 'D' };

            var price = cashier.Checkout(products);

            Assert.AreEqual(30, price);
        }

        private static List<IPricingStrategy> GetPricingStrategies()
        {
            return new List<IPricingStrategy>()
            {
                new PricingStategyA(),
                new PricingStrategyB(),
                new PricingStrategyC(),
                new PricingStrategyD()
            };
        }
    }
}
\$\endgroup\$
  • 2
    \$\begingroup\$ I've been wondering when I get to code review your work. :) My time is a little cramped this weekend, but i am going to try my hardest to review this now, hopefully by latest tomorrow (but a chance that I wont get it done till Monday) \$\endgroup\$ – Robert Snyder Jun 20 '15 at 23:20
  • \$\begingroup\$ No hurry @RobertSnyder. I'll be looking forward to it. \$\endgroup\$ – RubberDuck Jun 21 '15 at 1:09
  • \$\begingroup\$ God this is awful... what did I do?! Will wait for Robert's answer before I post a selfie. I want to see if he sees what I see. \$\endgroup\$ – RubberDuck Jun 21 '15 at 5:36
  • \$\begingroup\$ Geee! Do not use double for money! Only decimal. \$\endgroup\$ – Ognyan Dimitrov Jun 23 '15 at 12:29
5
\$\begingroup\$

1) Is my implementation of the strategy pattern correct?

Even semantically it is not. Consider

public class PricingStategyA : SaleStrategy
{
    public override Sku Sku { get; } = 'A';
    protected override double PricePerOne { get; } = 50;
    protected override double PricePerX { get; } = 130;
    protected override int X { get; } = 3;
}

PricingStategyA would apply only to A, it does not make sense to create this. Instead

var discounts = new Dictionary<string, SaleStrategy>()
{
    { "A", new SaleStrategy{ Price = 130.00m, Count = 3 }},
    { "B", new SaleStrategy{ Price = 45.00m, Count = 2 }}
};

or even

var discounts = new Dictionary<string, SaleStrategy>()
{
    { "HappyHour", new SaleStrategy{ Percent = 13.30m, Count = 3 }},
    { "YellowLabel", new SaleStrategy{ Percent = 25.00m, Count = 2 }}
};

In either case the should come from a db, feed or even text file and not be hardcoded. Like this you need to recompile/redeploy each time a new strategy is created *(even if C# 6 has Roslyn).


You should have a strategy for having decimal round off, ex:

Decimal.Round(value, 2, MidpointRounding.ToEven); 

Alternative would be to use BigInteger with "/100" on implemented internally, removing slightest chance of roundoff errors.

2) Was the strategy pattern a bad choice?

Yes, to me the Decorator pattern seems more applicable here.

Some discounts in real world are multiplicative, some additive others just apply highest one and for some they apply if 2 or more different items are bought together. It seems harder to implement.

3, 4) It feels like I defeated the purpose. Did I?

Yes, your gut feeling is right. You should sterilize your Sku implementation idea, as it should never reproduce any offsprings. In addition structs in c# had more than a few gotchas. For example readonly on a struct field is a lie.

One expects that a readonly field cannot change, but of course it can. "readonly" on a struct field is the declaration writing cheques with no money in its account. A struct doesn't own its storage, and it is that storage which can mutate.

Google "eric lippert immutable" if you want to know more, he has very good educational blog posts.

5) Should I go down a level and test the Strategies too, or instead of?

Yes. However note that testing first starts bottom-up, meaning you only use tested components and build upon them. In short this means not only you should do that, but you should do that before you start using specific strategies.

\$\endgroup\$
  • \$\begingroup\$ Can you elaborate on "You should sterilize your Sku implementation idea"? Also, I'm not sure that I agree testing first means writing things bottom up. That doesn't seem to be a requirement to me. Idk. \$\endgroup\$ – RubberDuck Jun 22 '15 at 9:11
  • \$\begingroup\$ Sure I can, Sku is reinventing the bicycle, it should be string or Guid+string. \$\endgroup\$ – Margus Jun 22 '15 at 9:19
  • \$\begingroup\$ Then I'm right back where I started. The idea was to make sure no one could accidentally assign a primitive type that wasn't a sku. Granted, I failed at that, but I can't see how switching to a plain string fixes my problem. \$\endgroup\$ – RubberDuck Jun 22 '15 at 10:01
  • \$\begingroup\$ If you have trust issues you use one of Java`s most notorious pattern called Builder pattern. You create private constructor so that class can only be instantiated inside a class and public static method usually named Of. This way you can even deny creation of the object if for example validation fails. For more about this read "Effective Java" by Joshua Bloch. There is adabtion to c# by Bill Wagner, everyone has their favorites. \$\endgroup\$ – Margus Jun 22 '15 at 11:15
  • \$\begingroup\$ I'm not sure you get what I'm saying. Let's say I use a plain string and I've got a msg variable. The idea is to provide some type safety by not allowing just any string being passed to something expected a Sku. Like I said, I failed at that goal, but none of what you're saying sounds particularly helpful with that goal. \$\endgroup\$ – RubberDuck Jun 22 '15 at 11:23
6
\$\begingroup\$

I'm going to try writing a short answer.

Structs are immutable. Not ifs, no buts.

A private field like this doesn't suggest immutability:

private char _value;

This does:

private readonly char _value;

If you can break encapsulation and do this:

public Sku(Sku sku)
{
    _value = sku._value;
}

Then you can break encapsulation and do that:

public MutateSku(ref Sku sku)
{
    sku._value = 42;
}

Where's the public char Value { get { return _value; } } property getter? I see why you don't need one...


SKU'se me sir, but that's not a SKU.

That Sku struct is essentially... a character. Sure it fulfills the A, B, C sample SKU's from the problem statement, but real-world SKU's could be 10-digit codes... your implementation simply doesn't scale all that well.

Does it make sense to treat any '%', '$' and ',' as a SKU? Absolutely not. There's no gain in using a Sku vs. using a char throughout your code!


I would have approached the data structure differently. First of all, I would have created a Product class, something like this:

public class Product
{
    public Product(string sku)
    {
        _sku = sku;
    }

    private readonly string _sku;
    public string Sku { get { return _sku; } }

    /* a real-life Product class would have other properties... */
    // private readonly string _description;
    // private readonly int _supplierId;

    /* Equals/GetHashCode based on _sku, and type being Product */
}

Then go on with ProductPricing strategies, without allowing just about any string out there to be treated as a Product.


Double is no money.

The correct type to use to talk cash, is decimal. By using double, you've exposed yourself to floating-point rounding errors, which is not cool for something granular like a SKU. I'm pretty sure at least one of your tests break if prices start involving non-integer amounts... and if not, then there's a false sense of accuracy going on here - using double for money is going to end up causing rounding errors one way or another, one day or another.

\$\endgroup\$
  • 1
    \$\begingroup\$ Bad, bad, bad ...BAD pun. \$\endgroup\$ – Mathieu Guindon Jun 22 '15 at 1:46
  • \$\begingroup\$ You caught me on the double and the private value not being ReadOnly, but I don't understand what you're getting at here. Where's the public char Value { get { return _value; } } property getter?. You're right about sku not being a sku, but that's why I created the struct. So I could swap it out for the real thing. I'm not picking up what you're laying down... Can you clarify why I'm getting no benefit? \$\endgroup\$ – RubberDuck Jun 22 '15 at 2:01
  • 1
    \$\begingroup\$ @RubberDuck a struct that exposes no type-specific members and equates and converts both implicitly and explicitly to a char begs the question "why?" in my mind, because you've made the type completely interchangeable with char. That's why I said you don't need that getter - the type being interchangeable with a char, its Value is just... itself. \$\endgroup\$ – Mathieu Guindon Jun 22 '15 at 2:09
  • 1
    \$\begingroup\$ Do you think it would be better if I got rid of the implicit conversion? I think that's where I went wrong. \$\endgroup\$ – RubberDuck Jun 22 '15 at 9:41
3
\$\begingroup\$

I'm going to disagree with everyone else about your use of the Sku struct.

Well done. It's exactly the sort of thing that should be encapsulated, it's an important domain entity in this line of business. In Domain Driven Design (and other books/blogs/authors) the concept of "primitive obsession" is often raised.

Let's take email as an example:

  • Emails are a limited subset of strings that fufill certain requirements

Almost every time I see this in an app it's like this:

void SendEmail (string email)
{
    if (email.IsValidEmailAddress())
    {
        // blah
    }
}

The validation isn't generally an extension method on string but you get my point. The validation starts cluttering up everywhere.

The same thing will happen with Sku if you're not careful. Let's pretend that a Sku is a string of length 1-15 containing only letters and numbers and the character '-'.

I would rather this were a class than a struct because a 15 char string is higher than MS guidelines for struct size.

// Represents a stock keeping unit.
public class Sku
{
    private readonly string _value;
    public string Value { get { return _value; } } 

    private Sku(string value)
    {
        _value = value;
    }

    private static bool IsValid(string value)
    {
        // validate
        return true;
    }

    public static implicit operator string(Sku sku)
    {
        return sku.Value;
    }

    public static explicit operator Sku(string value)
    {
        if (IsValid(value))
        {
            return new Sku(value);
        }
        throw new InvalidCastException();
    }
}

You could also add a parse method.

var sku1 = (Sku)"abc-9";
var sku2 = Sku.Parse("abCC");

string sku1AsString = sku1;
string sku2AsString = sku2;

Notice that the explicit operator can throw as not all strings are valid Skus but the implicit operator can't because all Skus are valid strings.

I feel like I've rambled, but hopefully something in the above is useful.

\$\endgroup\$
  • 1
    \$\begingroup\$ I'm glad somebody got what I was up to, but my implementation was bad. An implicit cast from Sku to char is fine, but from char to Sku was defeating the type safety. Thanks for mentioning the size issue. I had neglected that. \$\endgroup\$ – RubberDuck Jun 22 '15 at 13:30
  • \$\begingroup\$ But... making Sku a class over a struct changes everything ! \$\endgroup\$ – Mathieu Guindon Jun 22 '15 at 14:10
2
\$\begingroup\$

Right now, I would have to implement a new class for each sku. This would be impractical (to say the least) with hundreds of thousands of skus. Was the strategy pattern a bad choice? How could the code be made more resilient and re-usable over many skus that use the same strategy?

The strategy pattern was a great choice. You went wrong when you decided to use abstract base classes and ended up with child classes that look like this.

public class PricingStategyA : SaleStrategy
{
    public override Sku Sku { get; } = 'A';
    protected override double PricePerOne { get; } = 50;
    protected override double PricePerX { get; } = 130;
    protected override int X { get; } = 3;
}

public class PricingStrategyB : SaleStrategy
{
    public override Sku Sku { get; } = 'B';
    protected override double PricePerOne { get; } = 30;
    protected override double PricePerX { get; } = 45;
    protected override int X { get; } = 2;
}

That's insane. Insane. These values should be specified by the client code. This is exactly what constructors are for. Don't get so caught up in new syntax sugar that you write badly designed code. Make the SaleStrategy class a regular (non-abstract) class and give it a constructor that takes in these values. Also, while you're at it, do you really think that you're only ever going to have one sale strategy? No, you're going to have all kinds of different pricing strategies for items on sale, Buy One Get One, X% off, etc. So, give this one a better name.

public class XForYStrategy : IPricingStrategy
{
    private double _pricePerOne;
    private double _pricePerX;
    private int _x;

    public Sku Sku { get; }

    public XForYStrategy(Sku sku, double price, double pricePerX, int x)
    {
        _pricePerOne = price;
        _pricePerX = pricePerX;
        _x = x;
        Sku = sku;
    }

Now you can easily create new instances of the pricing strategy. i.e.

var strategy = new XForYStrategy('A', 2.50, 5, 3);

There's another problem though. In reality, a single sku can only have one pricing strategy at a time, but this implementation doesn't do anything to prevent that from happening. Also, why does a pricing strategy need to know what Sku it's attached to? Answer: It doesn't. You only did this so the Cashier knew which strategy to apply to a Sku. Kill two birds with one stone. Remove Sku from IPricingStrategy and provide the Cashier with a Dictionary<Sku, IPricingStategy instead of a List<IPricingStrategy.

public interface IPricingStrategy
{
    double GetPrice(int count);
}

public class Cashier
{
    private Dictionary<Sku, IPricingStrategy> _pricingStrategies;

    public Cashier(Dictionary<Sku, IPricingStrategy> pricingStrategies)
    {
        _pricingStrategies = pricingStrategies;
    }

    public double Checkout(IList<Sku> products)
    {
        double result = 0;
        foreach(var strat in _pricingStrategies)
        {
            var prods = products.Where(p => p == strat.Key);
            result = result + strat.Value.GetPrice(prods.Count());
        }

        return result;
    }
}

This guarantees the cashier only gets one strategy per sku and gives you the memory benefit of only needing different pricing strategies where the pricing strategy is actually different.

var oneNinetyNineStrategy = new RegularStrategy(1.99);
var twoFiftyStrategy = new RegularStrategy(2.50);

var dict = new Dictionary<Sku, IPricingStrategy>()
{
    {new Sku('A'), oneNinetyNineStrategy},
    {new Sku('B'), oneNinetyNineStrategy},
    {new Sku('C'), twoFiftyStrategy}
};

Now you could easily create a couple of classes to grab all of the strategies from a database, grab all of the strategy to sku mappings, and create this dictionary to pass off to the Cashier every day.

Now you're free to create as many pricing strategies as you need. Maybe your store wants to offer a "Buy 5 Get One Free" sale, or a "Buy One Get One" offer. You can do that without modifying any existing classes. Everything is closed to modification at this point.


It's also worth nothing that there's a bug in the Cashier.Checkout() method.

public double Checkout(IList<Sku> products)
{
    double result = 0;
    foreach(var strat in _pricingStrategies)
    {
        var prods = products.Where(p => p == strat.Sku);
        result = result + strat.GetPrice(prods.Count());
    }

    return result;
}

Just what would happen if the customer wants to purchase an item that there's no IPricingStrategy for? You'll give it away for free! You should take precautions to make sure this doesn't happen.

\$\endgroup\$
  • 2
    \$\begingroup\$ Good catch. You need a JustGiveItAwayPricingStrategy :-) \$\endgroup\$ – Mathieu Guindon Jun 22 '15 at 14:19
  • 1
    \$\begingroup\$ rats beat me to it.... you answered yourself in the 3rd person? Ha... funny \$\endgroup\$ – Robert Snyder Jun 22 '15 at 16:34
2
\$\begingroup\$

Since nobody else tackled your question of

Did I properly implement my Sku struct in regards to overriding Equals(), GetHashCode(), and overloading the operators?

I will do so. To make things short, I believe GetHashCode and the operator overloading looks pretty cut-and-dried correct. However, the Equals code:

public override bool Equals(object obj)
{
    if (obj == null) { return false; }

    Sku sku;
    try
    {
        sku = (Sku)obj;
    }
    catch
    {
        return false;
    }

    return _value == (char)sku;
}

violates a fundamental tenet of programming, that is: do not use try..catch for logic flow. Your entire Equals method can be replaced with:

public override bool Equals(object obj)
{
    var sku = obj as Sku?;

    return (sku != null) && ((Sku)sku)._value == _value;
}

Though, to be honest, I'm always a little skeeved out that I can access private members of the "other" object given that it's the same type. I know it's valid and allowed, just feels a bit off. I'd create apublic (or maybe internal) property called Value that gives well-defined access to the private member.

ETA: I'm feeling plucky today, so here's a bit of refactoring on Cashier for great justice. Someone mentioned moving from List<T> to IList<T>. I say go all the way to the most general of interfaces and make it IEnumerable<T> since no indexed access is required. Also LINQified the Checkout method to make it more about the what that's being done rather than the how:

public class Cashier
{
    private readonly IEnumerable<IPricingStrategy> _pricingStrategies;

    public Cashier(IEnumerable<IPricingStrategy> pricingStrategies)
    {
        _pricingStrategies = pricingStrategies;
    }

    public double Checkout(IEnumerable<Sku> products)
    {
        return _pricingStrategies
            .Select(strategy => new { strategy, prods = products.Where(p => p == strategy.Sku) })
            .Select(strategyProduct => strategyProduct.strategy.GetPrice(strategyProduct.prods.Count()))
            .Sum();
    }
}
\$\endgroup\$
  • \$\begingroup\$ Thanks for the reminder about nullable types. I find that access to the private field is a little more palpatable if you split the expression into an if null then return false...return expression type of statement. \$\endgroup\$ – RubberDuck Jun 22 '15 at 17:50
1
\$\begingroup\$

My turn: typeo in your class name PricingStrategyA <- you were missing the 'r' in Strategy.

I also see that you are hung up on having a pricing strategy for every product. And although you could do this it doesn't make much sense in the market world. The old adage "Everything has a price" comes into play on this one. From a cash registers standpoint all SKUs have a price. That should be part of your struct. However, even struct is a poor choice IMO. Struct should be able to describe a value in the end. Instead a SKU and a price is closer to key value pair. What's more, and I'm sure all store owners would agree, I wouldn't want to be able to enter in a product into my database without giving it a price, yet by default you can, and you do as is shown in your test with var products = new List<Sku>() { 'E', 'E' };

Next up is that I feel with your improved idea about how to create strategies is still wrong. Why would you create a set price for items then attach a sku to each of those prices. It is like you are switching your key and your value around. Now your entire system is indexed off of the price instead of the Sku.

TestCaseData. I know you are using MSTest, but you've preached about using that ability from NUnit a few times. Not using it with the way you wrote your tests was like holding one hand behind your back while coding. Why? Even if MSTest doesn't support it you can write you test in such a way that at least gets rid of the duplicate code. (just saw another typo Cashier_TwoC_Is20.. should be Cashier_TwoC_Is40) So you could delete all your other methods and replace it with just this one. And if you wanted your error code to be more specific you could change the assert part to check for a "error string"

private class SkuDataSource
{
    public IList<Sku> Products { get; private set; }
    public int ExpectedPrice { get; private set; }
    public SkuDataSource()
    {
        Products = new List<Sku>();
    }
    public SkuDataSource WithProducts(params Sku[] skus)
    {
        foreach (var sku in skus)
            Products.Add(sku);

        return this;
    }

    public SkuDataSource ShouldCost(int expectedPrice)
    {
        ExpectedPrice = expectedPrice;

        return this;
    }
}

That allows me to write this as a test instead of the 16+ tests that all do and say the same thing.

[TestMethod]
public void WithGivenSetOfProducts_CashierWillReturnExpectedPrice()
{
    var cashier = new Cashier(GetPricingStrategies());
    var skuDataSources = new[]
    {
        new SkuDataSource().WithProducts().ShouldCost(0),
        new SkuDataSource().WithProducts('A').ShouldCost(50),
        new SkuDataSource().WithProducts('A','A').ShouldCost(100),
        new SkuDataSource().WithProducts('A','A','A').ShouldCost(130),
        new SkuDataSource().WithProducts('A','A','A','A').ShouldCost(180),
        new SkuDataSource().WithProducts('A','A','A','A','A','A').ShouldCost(260),
        new SkuDataSource().WithProducts('B').ShouldCost(30),
        new SkuDataSource().WithProducts('B','B').ShouldCost(45),
        new SkuDataSource().WithProducts('B','B','B').ShouldCost(75),
        new SkuDataSource().WithProducts('B','B','B','B').ShouldCost(90),
        new SkuDataSource().WithProducts('A','B').ShouldCost(80),
        new SkuDataSource().WithProducts('B','A', 'B').ShouldCost(95),
        new SkuDataSource().WithProducts('C').ShouldCost(20),
        new SkuDataSource().WithProducts('C','C').ShouldCost(40),
        new SkuDataSource().WithProducts('D').ShouldCost(15),
        new SkuDataSource().WithProducts('D','D').ShouldCost(30),
    };

    foreach (var dataSource in skuDataSources)
    {
        var price = cashier.Checkout(dataSource.Products);
        var productsJoined = string.Join(",", dataSource.Products);
        Assert.AreEqual(dataSource.ExpectedPrice, price, $"The expected price for [{productsJoined}] skus should have been {dataSource.ExpectedPrice}");
    }
}

Now to address the final part of the system that both this and the other systems missed. Are you telling me that the ONLY way to put in discounts into the system is to rebuild and redeploy your entire checkout process???? that to me seems more absurd than having to write out individual pricing strategies for each sku.

Back to using a struct for a SKU. You can simplify your Equals methods greatly by implementing IEquatable<Sku>. By doing so you can easily see a that checking for null is not needed.

    public override bool Equals(object obj)
    {
        return Equals(obj as Sku); //compiliation error here
    }

    public bool Equals(Sku other)
    {
        return other._value == _value;
    }

but that results in a compile error because structs can't be null! which means that by you checking for null you are wasting cycles. so delete your null checks and cast the object as a Sku leaving you with

public override bool Equals(object obj)
{
    return Equals((Sku)obj);
}

public bool Equals(Sku other)
{
    return _value == other._value;
}

and that is all the new things that I can see at this moment. You and everyone else covered the parts I wanted to talk about.

\$\endgroup\$
  • \$\begingroup\$ I work in grocery. You actually can and do want to enter new skus into the system before they're priced. =;)- Thanks for the tip about IEquatable. \$\endgroup\$ – RubberDuck Jun 22 '15 at 18:32
  • \$\begingroup\$ One thing I don't understand, you mention that with the new method I proposed, I'd have to recompile to enter new prices. I don't think that's true. There could be a class that picks up the data from an external source and passes off a dict to the cashier. Am I missing something? (Perhaps a follow up is in order...) \$\endgroup\$ – RubberDuck Jun 22 '15 at 18:34
  • \$\begingroup\$ Ohhhh. I'm picking up what you're laying down now. Yes. If we wanted to do some kind of promotion that the system didn't yet support, it would have to be redeployed. Thankfully, the number of different kinds of promotions are limited. I can honestly only think of 10 at the most. Point taken though. \$\endgroup\$ – RubberDuck Jun 22 '15 at 18:41
  • \$\begingroup\$ meh sorta. You have the right mindset about using another class to read some sort of external file. As it stands right now though you have no interface defined to allow for that. And even if you did then you'd have to (or want to) move your pricing strategy to your test project as a mock pricing strategy provider of sorts. \$\endgroup\$ – Robert Snyder Jun 22 '15 at 19:03
  • \$\begingroup\$ As for working in grocery, I'd be curious what part. It sounds more like from an inventory point of view. In which case, yes I can see where you are coming from. From the cashiers point of view though everything you scan in has a price set already, and if it wasn't you'd probably get an invalid SKU error on your screen. Atleast that is what happened to me at the grocery store I was a cashier for. \$\endgroup\$ – Robert Snyder Jun 22 '15 at 19:05

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.