5
\$\begingroup\$

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); } } public class Basket { private readonly IDictionary<int, IDiscountStrategy> _discountStrategies; private readonly IList<HashSet<Book>> _bookSets; public Basket(IDictionary<int, IDiscountStrategy> discountStrategies) { _discountStrategies = discountStrategies; _bookSets = new List<HashSet<Book>>(); } public void AddBook(Book 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) { _bookSets[setIndex].Add(book); 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 { private readonly Basket _basket; public BookTest() { var discountStrategies = new Dictionary<int, IDiscountStrategy> { {1, new NoDiscount() }, {2, new FivePercentDiscount() }, {3, new TenPercentDiscount() }, {4, new FifteenPercentDiscount() }, {5, new TwentyPercentDiscount() }, }; _basket = new Basket(discountStrategies); } [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 _basket.AddBook(new Book("Volume 1")); _basket.AddBook(new Book("Volume 1")); var expected = 16; //act var actual = _basket.Checkout(); //assert Assert.Equal(expected, actual); } [Fact] public void should_have_five_percent_discount_when_two_books_books_do_not_have_the_same_volume() { //arrange _basket.AddBook(new Book("Volume 1")); _basket.AddBook(new Book("Volume 2")); var expected = 16 * .95; //act var actual = _basket.Checkout(); //assert Assert.Equal(expected, actual); } [Fact] public void should_have_ten_percent_discount_when_three_books_do_not_have_the_same_volume() { //arrange _basket.AddBook(new Book("Volume 1")); _basket.AddBook(new Book("Volume 2")); _basket.AddBook(new Book("Volume 3")); var expected = 24 * .90; //act var actual = _basket.Checkout(); //assert Assert.Equal(expected, actual); } [Fact] public void should_have_fifteen_percent_discount_when_four_books_do_not_have_the_same_volume() { //arrange _basket.AddBook(new Book("Volume 1")); _basket.AddBook(new Book("Volume 2")); _basket.AddBook(new Book("Volume 3")); _basket.AddBook(new Book("Volume 4")); var expected = 32 * .85; //act var actual = _basket.Checkout(); //assert Assert.Equal(expected, actual); } [Fact] public void should_have_twenty_percent_discount_when_four_books_do_not_have_the_same_volume() { //arrange _basket.AddBook(new Book("Volume 1")); _basket.AddBook(new Book("Volume 2")); _basket.AddBook(new Book("Volume 3")); _basket.AddBook(new Book("Volume 4")); _basket.AddBook(new Book("Volume 5")); var expected = 40 * .8; //act var actual = _basket.Checkout(); //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 _basket.AddBook(new Book("Volume 1")); _basket.AddBook(new Book("Volume 2")); _basket.AddBook(new Book("Volume 3")); _basket.AddBook(new Book("Volume 3")); var expected = (24 * .9) + 8; //act var actual = _basket.Checkout(); //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 _basket.AddBook(new Book("Volume 1")); _basket.AddBook(new Book("Volume 2")); _basket.AddBook(new Book("Volume 3")); _basket.AddBook(new Book("Volume 3")); _basket.AddBook(new Book("Volume 3")); var expected = (24 * .9) + 16; //act var actual = _basket.Checkout(); //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 _basket.AddBook(new Book("Volume 1")); _basket.AddBook(new Book("Volume 2")); _basket.AddBook(new Book("Volume 3")); _basket.AddBook(new Book("Volume 4")); _basket.AddBook(new Book("Volume 5")); _basket.AddBook(new Book("Volume 6")); _basket.AddBook(new Book("Volume 7")); _basket.AddBook(new Book("Volume 8")); _basket.AddBook(new Book("Volume 9")); _basket.AddBook(new Book("Volume 10")); _basket.AddBook(new Book("Volume 11")); _basket.AddBook(new Book("Volume 11")); var expected = (40 * .8) + (40 * .8) + 16; //act var actual = _basket.Checkout(); //assert Assert.Equal(expected, actual); } }

Thanks in advance!

\$\endgroup\$
2
  • 1
    \$\begingroup\$ 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! \$\endgroup\$ Commented Apr 21, 2020 at 23:32
  • 2
    \$\begingroup\$ @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) \$\endgroup\$ Commented Apr 22, 2020 at 0:25

4 Answers 4

Reset to default
4
\$\begingroup\$

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

Basket

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.

\$\endgroup\$
5
+25
\$\begingroup\$

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).
\$\endgroup\$
3
\$\begingroup\$

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.

\$\endgroup\$
1
  • \$\begingroup\$ Bump for Decimal. \$\endgroup\$ Commented Apr 23, 2020 at 13:39
2
\$\begingroup\$

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.

\$\endgroup\$
0

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.