patterncsharpModerate
TDD Supermarket Pricing Kata
Viewed 0 times
katatddsupermarketpricing
Problem
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:"
"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,
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,
Solution
ArgumentExceptions
You're null checking arguments! Awesome!
You even remembered the test for it! Even better!
But what about this?
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.
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.
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.
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.");
}Code Snippets
public void AddPricingRule(PricingRule rule)
{
if (rule == null)
{
throw new ArgumentNullException();
}public void AddItem(Item item)
{
items.Add(item);
}if (rule == null)
{
throw new ArgumentNullException();
}
if (string.IsNullOrWhiteSpace(rule.ItemName))
{
throw new ArgumentException();
}if (rule == null)
{
throw new ArgumentNullException("rule");
}
if (string.IsNullOrWhiteSpace(rule.ItemName))
{
throw new ArgumentException("rule.ItemName cannot be null or whitespace.");
}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.");
}Context
StackExchange Code Review Q#93732, answer score: 10
Revisions (0)
No revisions yet.