# Harry Potter Kata

I have implemented the Harry Potter Kata and I need your feedbacks.

The rules are:

A book costs 8 euros. There are 5 different volumes. To get a discount, you must buy books of different volumes:

• Buying 1 book doesn't give you a discount
• Buying 2 books applies a 5% discount
• Buying 3 books applies a 10% discount
• Buying 4 books applies a 15% discount
• Buying 5 books applies a 20% discount

Examples:

• Given a basket When I buy 2 books of volume 1 Then the total is 16 euros
• Given a basket When I buy 1 book of volume 1 And I buy 1 book of volume 2 Then the total is 15.2 euros

The implementation:

 public class Book
{
public double Price { get; } = 8;
public string Volume { get; }

public Book(string volume)
{
Volume = volume;
}

public override bool Equals(object obj)
{
// Is null?
if (obj is null)
{
return false;
}

// Is the same object?
if (ReferenceEquals(this, obj))
{
return true;
}

// Is the same type?
return obj.GetType() == this.GetType() && IsEqual((Book)obj);
}

public override int GetHashCode()
{
return HashCode.Combine(Price, Volume);
}

private bool IsEqual(Book book)
{
// A pure implementation of value equality that avoids the routine checks above
// We use String.Equals to really drive home our fear of an improperly overridden "=="
return string.Equals(Volume, book.Volume)
&& Equals(Price, book.Price);
}
}

{

{
_discountStrategies = discountStrategies;
_bookSets = new List<HashSet<Book>>();
}

{
var setIndex = 0;
var inserted = false;

while (!inserted)
{
if (_bookSets.Count <= setIndex) _bookSets.Add(new HashSet<Book>());

if (!_bookSets[setIndex].Contains(book) && _bookSets[setIndex].Count < _discountStrategies.Count)
{
inserted = true;
}
setIndex++;
}
}

public double Checkout() => _bookSets.Sum(set =>
GetTotalCostBeforeDiscount(set) * _discountStrategies[set.Count].GetDiscount());

private double GetTotalCostBeforeDiscount(IEnumerable<Book> books) => books.Sum(b => b.Price);
}

public class FifteenPercentDiscount : IDiscountStrategy
{
public double GetDiscount() => .85;
}
public class FivePercentDiscount : IDiscountStrategy
{
public double GetDiscount() => .95;
}
public class NoDiscount : IDiscountStrategy
{
public double GetDiscount() => 1;
}
public class TenPercentDiscount : IDiscountStrategy
{
public double GetDiscount() => .9;
}
public class TwentyPercentDiscount : IDiscountStrategy
{
public double GetDiscount() => 0.8;
}
public interface IDiscountStrategy
{
double GetDiscount();
}

public class BookTest
{
public BookTest()
{
var discountStrategies = new Dictionary<int, IDiscountStrategy>
{
{1, new NoDiscount() },
{2, new FivePercentDiscount() },
{3, new TenPercentDiscount() },
{4, new FifteenPercentDiscount() },
{5, new TwentyPercentDiscount() },
};

}
[Fact]
public void should_book_price_equals_to_8_when_created()
{   //arrange
var book = new Book("Volume 1");
var expected = 8;
//act
var actual = book.Price;
//assert
Assert.Equal(expected, actual);
}

[Fact]
public void should_two_books_price_equals_to_16_when_they_have_the_same_volume()
{
//arrange
var expected = 16;
//act
//assert
Assert.Equal(expected, actual);
}
[Fact]
public void should_have_five_percent_discount_when_two_books_books_do_not_have_the_same_volume()
{
//arrange
var expected = 16 * .95;
//act
//assert
Assert.Equal(expected, actual);
}
[Fact]
public void should_have_ten_percent_discount_when_three_books_do_not_have_the_same_volume()
{
//arrange
var expected = 24 * .90;
//act
//assert
Assert.Equal(expected, actual);
}
[Fact]
public void should_have_fifteen_percent_discount_when_four_books_do_not_have_the_same_volume()
{
//arrange
var expected = 32 * .85;
//act
//assert
Assert.Equal(expected, actual);
}
[Fact]
public void should_have_twenty_percent_discount_when_four_books_do_not_have_the_same_volume()
{
//arrange
var expected = 40 * .8;
//act
//assert
Assert.Equal(expected, actual);
}

[Fact]
public void should_have_ten_percent_discount_when_three_books_do_not_have_the_same_volume_and_no_discount_for_the_last_book()
{
//arrange
var expected = (24 * .9) + 8;
//act
//assert
Assert.Equal(expected, actual);
}
[Fact]
public void should_have_ten_percent_discount_when_three_books_do_not_have_the_same_volume_and_no_discount_for_the_last_two_books()
{
//arrange
var expected = (24 * .9) + 16;
//act
//assert
Assert.Equal(expected, actual);
}

[Fact]
public void should_create_three_sets_and_the_max_size_of_a_set_equal_to_the_number_of_strategies_to_not_get_KeyNotFoundException()
{
//arrange
var expected = (40 * .8) + (40 * .8) + 16;
//act
//assert
Assert.Equal(expected, actual);
}
}


• Hi ilyes, I am not such skilled to do full code review, however I have a few comments that might help you. The Checkout() method has two lines. I would use old style of method with curly braces instead of =>. I would rename Checkout() method to something more self-explanatory, e.g. GetTotalCostAfterDiscount() or _basket.Calculate(). In your first test method, I would check one book basket calculation with the same concept as used in all other tests. When reading this lambda .Sum(set => I was firstly confused, that set is some keyword. I would use s =>. I like your solution! – Tomas Paul Apr 21 '20 at 23:32
• @TomasPaul Please add an answer instead of a comment. Refer to the section When shouldn't I comment? on Comment everywhere. (hint: you can possibly earn more reputation that way) – Sᴀᴍ Onᴇᴌᴀ Apr 22 '20 at 0:25

It is good practice to work with immutable classes. Most of your namings are clear.

# Algorithm

It took me a while to understand the logic. I understand that the algorithm is: dividing the books to groups where each group doesn't contain the same volume twice and has a max size of 5. Create larger groups as possible in order to maximize the discount.

I would add this description as a comment.

The function responsible for the dividing should be called something like DivideToUniqeVolumeGroups.

## Pseudo Code

Here is a pseudo-code of your algorithm:

For each book:

1. Find a non-full group which doesn't contain a book with a given volume

2. If such group don't exists create a new group

3. Add the book to the group

I think writing the code to be similar to the pseudo-code above will be much more readable.

# Hidden Assumptions

You are looking for a group that doesn't contain a specific volume, but in code you use !_bookSets[setIndex].Contains(book). It is not clear. It is working because all books have the same price and it has only those 2 properties. Once something will change it will stop working.

# Naming

Usually in shopping systems what you called basket is called a cart.

I like to name a Dictionary with a name that explains what is the key and what is the value. In that case booksCount2discount.

# Strategy Design Pattern

The strategy should get a books list in order to create logic depending on the books.

I think that the strategy design pattern is a bit over-engineering in this case because the logic is always the same: applying a discount according to the books count. I think a dictionary between the books count and the discount value is sufficient.

You created a class for each discount value. Don't need for all those classes, replace it with a single class that accepts the discount value as a parameter in a constructor. This way the discounts can be configurable.

It is more clear if the strategy returns the discount value. (0.1 for 10% discount)

# Code

Move the logic from AddBook to Checkout. I think it is more clear that way. Also, you will not need _bookSets.

Creating a class for a group called UniqeVolumeGroup will add to readability. Adding validations will help to find bugs faster.

I think GetTotalCostBeforeDiscount is unnecessary since the code is quite clear and shorter than the function name.

## Book

Don't set the price inside Book.

# Tests

You are using a class member _basket in all the tests. Depending on the testing framework, this could lead to tests affecting each other when running in parallel. I would create a new basket for each test.

should_create_three_sets_and_the_max_size_of_a_set_equal_to_the_number_of_strategies_to_not_get_KeyNotFoundException This name is too technical because it mentions strategies and exceptions. You should name the tests in terms of the domain. I would call the test something like group size should not be greater than max books at a discount.

Some people have already pointed out important points, particularly the use of Decimal for money values.

Higher-level stuff:

• You have some white-space inconsistencies.
• Having the basket start with a batch of possibly applicable discounts doesn't make much sense. The basket can more-or-less just be a list until the time comes to calculate discounts. Hopefully this ends up being less verbose.
• In general, a little less verbosity would be nice. Book.Equals for example could be single and block.
• Instead of having Basket.AddBook, I would prefer to construct the baskets with books already in them. In the simplest case, public Basket(params IEnumerable<Book> book_lists){...} would allow a lot of flexibility including merging existing baskets. In general, one usually can and should avoid changing the state of objects.
• While building a dictionary in which to look up discount strategies is efficient, it's not what I would want to see in production code; it's too limiting. I've worked with a couple different "discount" paradigms, and it's a good idea to build in flexibility early on. If you get it right, then it's simpler in the long run than adding complexity as you go. Some common patterns:
• A discount object should be able to inspect the basket and report if it can be applied.
public bool Applicable(Basket basket){...}
• Can multiple discounts apply? Will this ever depend on which discounts might apply? Have a system for figuring this out.
• Discount objects should have a property (often just an int, whatever) to denote the order in which they should apply (for example 0.9 * (x - 5) is different from (0.9 * x) - 5).

First off, anything pertaining to money or currency should use Decimal instead of Double. Decimal is a base-10 floating point for exact decimal places, whereas Double is an approximation.

Book class should implement IEquatable<Book>.

The discount strategies should be static or overall strategies independent of any given Basket. Perhaps this would even be in a separate class. When you later calculate what's in a given Basket, you would then lookup and apply the appropriate strategy based on the Basket contents at that given point in time.

• Bump for Decimal. – ShapeOfMatter Apr 23 '20 at 13:39

I would like to put just few recommendation or considerations, not full code review evaluating design patterns or functional issues.

• The Checkout() method has two lines. I would use old method body style with curly braces instead of expression body defnition with lambda operator =>.
• I would rename Checkout() method to something more self-explanatory, e.g. GetTotalCostAfterDiscount() or _basket.Calculate().
• In your first test method, I would check one book basket calculation with the same concept as used in all other tests. Or you can add test with no added books (boundary value tests).
• When reading this lambda .Sum(set => I was firstly confused, that set is some keyword. I would personally use s =>.

I like your solution, I cannot see any major issues. It is neat, following SOLID principles and naming conventions. Thanks.

• Hi Tomas,thanks for your feedback, I appreciate it :). – ilyes Apr 22 '20 at 9:34