Code Kata: KataPotter Solution Critique Request

[Bobby Johnson]

I have been working on the KataPotter code kata over at Coding Dojo for the last couple weeks (spending a few hours a week on it.) and I was able to complete it today. Here is the kata and here is my solution. I was using this kata as a starting place to try to force myself to learn to use MSpec effectively, so any comments or criticism on my specs or my solution would be greatly appreciated.

I also discovered something pretty awesome about Resharper. If you have both the XUnit and MSpec plugins installed, R# will run all unit tests in the solution as one session even with a mixed assembly which is really awesome.

--
"The explanation requiring the fewest assumptions is most likely to be correct."

- Occam’s Razor
http://en.wikipedia.org/wiki/Occam's_Razor

[Simone Busoli]

I did it now and followed a different route which I think brings to a simpler solution. Instead of asking a discount if a set of books satisfies it, why don't you try grouping books in a way that the grouping provides the best discount?

[Bobby Johnson]

You are the second person that I have discussed this with to mention grouping the books. I may be to close to my solution to see it yet. I'll have to let it percolate for a few days.

[Simone Busoli]

If you were the cashier, how would you calculate it? That's your solution :)

[Bobby Johnson]

thanks for the pointer, Ill give it another go sometime in the coming weeks.

[Ralf Westphal]

Are you sure you´ve solved the kata correctly?
What´s your result for a cart with quantities (2, 2, 2, 1, 1) for 5
different titles if each costs 8 USD and the discounts are 5%, 10%,
20% 25%?

I looked at your code and got lost. So many classes, so many
statements. But where´s actually the solution to the discount
calculation problem?
Why would you need a book or even a cart class?

-Ralf

PS: I´m just today running a clean code developer workshop where I
posed this kata as an excersise. And I´m seeing the participants
running into the same kind of overengineering.

On Oct 19, 11:30 pm, Bobby Johnson <bobby.john...@gmail.com> wrote:
> I have been working on the KataPotter code kata over at Coding Dojo for the
> last couple weeks (spending a few hours a week on it.) and I was able to
> complete it today. Here is the

> kata<http://codingdojo.org/cgi-bin/wiki.pl?KataPotter>and here is my
> solution <http://github.com/NotMyself/Practice-Code-Katas>. I was using this

> kata as a starting place to try to force myself to learn to use MSpec
> effectively, so any comments or criticism on my

> specs<http://github.com/NotMyself/Practice-Code-Katas/blob/master/Katas/Kat...>or

[Bobby Johnson]

Hi Ralf,

Thank you for for feedback.

I am not positive I solved the kata completely, but all of my specifications pass including the example you mentioned.

 [Subject("calculating discount prices")]
    public class when_calculating_the_price_of_the_kata_example_set_of_books : with_discounts
    {
         Establish context = () =>
                                        {
                                            cart.AddBook(new Book("Book 1", 8));
                                            cart.AddBook(new Book("Book 1", 8));
                                            cart.AddBook(new Book("Book 2", 8));
                                            cart.AddBook(new Book("Book 2", 8));
                                            cart.AddBook(new Book("Book 3", 8));
                                            cart.AddBook(new Book("Book 3", 8));
                                            cart.AddBook(new Book("Book 4", 8));
                                            cart.AddBook(new Book("Book 5", 8));
                                            correctPrice = 51.2M;
                                        };

         Because of = () => { price = calculator.CalculatePriceFor(cart); };

         It should_calculate_the_correct_price = () => price.ShouldEqual(correctPrice);
    }


The method that solves the calculation problem is located in the Discounter class, I have pasted it here.

public IList<Book> ApplyDiscounts(IList<Book> books)
    {
      var discounts = _locator.GetDiscountsFor(books)
        .OrderBy(x => x.Percentage);

      var bestDiscountedBooks = books;

      for(int i = 1; i <= discounts.Count(); i++)
      {
       var testDiscountBooks = books.Clone();

        foreach(var discount in discounts.Take(i).OrderByDescending(x => x.Percentage))
        {
          while(discount.IsSatisfiedBy(testDiscountBooks))
            discount.Apply(testDiscountBooks);
        }

        if(testDiscountBooks.Sum(x => x.Price) < bestDiscountedBooks.Sum(x => x.Price))
          bestDiscountedBooks = testDiscountBooks;
      }

      return bestDiscountedBooks;
    }

The Book and Cart class served two purposes for me in this exercise. Having never done this particular kata before, I needed a place to start. I tend to start with a very literal interpretation of the specifications. The kata talks about books, baskets and calculations so, they were the first three classes I created. I simply worked outward from there. They gave me a conceptual model of the space I was working in.

Sorry you got lost. I try for expressiveness and clarity in my code, so that is a bit distressing. I think this is a byproduct of this being the first time I've done the kata. While working on it, I was lost as well. ;) I had no idea how I was going to solve it. So, I took a very literal brute force approach. I didn't quite understand how each piece was going to fit together so I clung to SRP and Interface Segregation in case I needed to completely move things around.

You would have really been lost if you saw the code last week when I had concrete implementations of FivePercentDiscount, TenPercentDiscount, TwentyPercentDiscount, TwentyFivePercentDiscount, TwoBookSpecification, ThreeBookSepecification.. and so on. I collapsed all those classes down to a Discount class and a DiscountRepository that basically acted like a factory for the specific discounts.

Given my understanding of the solution I arrived at, I can simplify it by collapsing the Discounter and DiscountLocator classes into the PriceCalculator class. I could also, drop the Cart class all together because at the end of the exercise it was only syntactical sugar. It could easily be replaced by just a IList<Book>. The DiscountRepository class is also mostly unneeded. Makes testing nice though.

So, the feedback I have received so far:

• There is another simpler solution that involves grouping the books (think of what the cashier would do)
• My solution was over-engineered and confusingly hard to follow
• Code Metrics tell me I have a Maintainability Index of 86, Cyclometer Complexity of 63 and 78 Lines of code.
  

[Al Tenhundfeld]

Bobby, 

First, thanks for sharing. It's not easy to put code out there for everyone to critique, but it's essential for our craft to evolve. 

General feedback:

I too felt the solution was a bit hard to follow. I also tend to favor lots of small classes. However, in this solution, it feels like there are a lot of classes that don't do much, while the meat (or core domain) of the solution is in a single method with nested loops (Discounter.ApplyDiscounts). 

Specific feedback:

1) I don't like the convention of putting the interface and implementation in the same file, and I really don't like the convention of naming that file after the interface. This may just be a personal style thing, but for me, it detracts from the clarity. 

2) For the purposes of keeping the exercise simple, I would consider removing the interfaces and making the methods you want to mock virtual. RhinoMocks should be able to handle that. 

3) I dislike that the Discounter actually modifies the book price. The result price is a magic number with no traceability. I'd rather see a solution where the book has a separate discount amount field or a solution where discounts are added to the cart/order. Ideally, while debugging, I'd easily be able to determine a book's original price, the book's discount/discounted price, and why the book was discounted. (This might be over-engineering, but when money is concerned, I consider traceability an essential requirement.)

4) I might just be missing something, but I don't see any tests that are focused on Discounter. If so, I consider that to be a significant flaw. Discounter.ApplyDiscounts is the most complicated method.

Kinds regards,

Al

[Oliver Seiler]

I got intrigued on reading the thread and looking through the links.
Hadn't seen the problem before, so did an Erlang solution this morning
that passes all of the tests on codingdojo.org link. It comes in under
a 100 lines of non-test code and is, I think, reasonably clear, and
quite possibly correct. If anyone is interested I can put it up here
in a followup.

On Mon, Oct 19, 2009 at 4:35 PM, Bobby Johnson <bobby....@gmail.com> wrote:
> thanks for the pointer, Ill give it another go sometime in the coming weeks.
>
> On Mon, Oct 19, 2009 at 4:30 PM, Simone Busoli <simone...@gmail.com>
> wrote:
>>
>> If you were the cashier, how would you calculate it? That's your solution
>> :)
>>
>> On Tue, Oct 20, 2009 at 01:27, Bobby Johnson <bobby....@gmail.com>
>> wrote:
>>>
>>> You are the second person that I have discussed this with to mention
>>> grouping the books. I may be to close to my solution to see it yet. I'll
>>> have to let it percolate for a few days.

This seems to be key to dealing with the optimization side of this
problem (short of doing it brute-force).

[Fred Ballard]

Please do put it up here.

[Simone Busoli]

I did it in C#, can post that too

[Oliver Seiler]

On Wed, Oct 21, 2009 at 2:43 PM, Fred Ballard <fred...@gmail.com> wrote:
> Please do put it up here.

> [...]

Attached.

For erlang newbs (those with slightly less experience than myself),
with erlang installed:

oseiler@oseiler ~/work/priv/katapotter $ erl
Erlang R13B02 (erts-5.7.3) [source] [64-bit] [smp:4:4] [rq:4] [async-threads:0]

Eshell V5.7.3  (abort with ^G)
1> c(katapotter).
{ok,katapotter}
2> katapotter:test().
 All 29 tests passed.
ok
3> katapotter:price([2, 2, 2, 1, 1]).
38.4
4> q().

It doesn't actually care what the numbers are, and can take strings as
well. Things work fine as long as there aren't more than 5 distinct
values:

9> katapotter:price(['half-blood prince', 'philosophers stone']).
15.2

Note that these aren't strings in Erlang; they're atoms. In this
context using strings (double-quotes) doesn't change the behaviour,
however.

10> 8*5*0.75.
30.0
11> katapotter:price([0,1,2,3,4]).
30.0
12> katapotter:price([1,2,3,4,5]).
30.0
13> katapotter:price([0,1,2,3,4,5]).
48

Based on the problem statement that last result is undefined, but one
could argue that it should be 38. I'm not worrying about this myself,
however, because then I'd need to figure out what [0,1,2,3,4,5,6]
should do (46, I guess).

As for the code, I'm not completely happy with some of the naming. The
logic around sort_by_cost_for_next_book is a bit complicated, but I
hope the intent is clear. This is pretty much the most important part
of this solution.

Cheers
Oliver

PS. Made some edits to the source in the process of typing this, and
I'm not entirely sure that gmail got the attachment correct, but the
important bits should be there.

[Simone Busoli]

And here's mine, no much explanation needed I guess http://gist.github.com/215545

[Bobby Johnson]

holy crap Simone, I didn't want to look to much because I want to figure out the grouping thing myself, but what i did look at was quality stuff. Thanks for posting it.

[Simone Busoli]

Thanks Bobby, but now you'll need to find a different kata to perform :)

Ok some observations on my performance I'd be glad to get feedback on:

- I didn't do it strictly TDD, that is, I made the choice to find the right algorithm very soon, instead of writing just as little code to make the test green

- It's basically a hack, most times you cannot afford to test the outcome of an operation before performing it, therefore other solutions are welcome

[Steven Smith]

I have a very rough version done at our Software Craftsmanship meeting
in Hudson, Ohio (http://HudsonSC.com) with a partner posted as well.
It's TDD driven and was timeboxed so it would obviously need cleaned
up, but it's a "real" example of where the tests lead us (along with
YAGNI and trying to build the simplest thing that would get the tests
to pass):
http://stevesmithblog.com/blog/a-first-pass-at-potterkata/

Thanks again Bobby for the idea to try this kata.

Steve

--
Steve Smith
http://SteveSmithBlog.com/
http://twitter.com/ardalis

[Jeff Morgan]

Like Steve I was also at the meeting but I was unable to complete the kata
at the meeting. I finally found a few cycles to complete it this evening. I
put it in github if yo wish to have a look.

http://github.com/cheezy/KataPotter

-Cheezy

[Doug Bradbury]

I was inspired by everyone's work and did the kata in Ruby.

Here is my solution. It turns out to look a lot like Simone's.
http://github.com/dougbradbury/potter_kata

I realized something in doing this kata. I think there is an
incorrect assumption that when doing TDD, you don't have to think
about the algorithm at all.

I played dumb all the way up to the last test that requires you to
optimize the groupings, and then I completely rewrote the algorithm.
I extract two classes along the way, TDDing each class as I discovered
that I needed it.

What approaching the algorithm test first did for me was to help me
reason about it. I started out with no idea how to solve it. By
working through the easy cases, I got to understand the domain. I
left it overnight and in the morning I had the pieces put together in
my head.

TDD changes the way we THINK about code.

Doug

[Sander Kooijmans]

Hi Simone,

I am sorry to inform you that your solution is not correct.

I added the following test:

            Assert.AreEqual(78.8, Price(0, 0, 0, 1, 1, 2, 2, 2, 2, 3, 3, 4));

This test fails, since your program comes up with the price 86.4 instead of 78.8. Your program comes up with the cart [0,1,2], [0,1,2], [0,2,3], [2,3,4].
To get 78.8 you should have the cart [2], [0,2], [0,1,2,3], [0,1,2,3,4].

[Simone Busoli]

Wow Sander, that was timely feedback, but you're indeed correct! You might want to have another look at the gist though, pasted here for your convenience ;) https://gist.github.com/215545

Simone

--

You received this message because you are subscribed to the Google Groups "software_craftsmanship" group.

To post to this group, send email to software_cr...@googlegroups.com.
To unsubscribe from this group, send email to software_craftsma...@googlegroups.com.
Visit this group at http://groups.google.com/group/software_craftsmanship?hl=en.
For more options, visit https://groups.google.com/groups/opt_out.
 
 

[Sander Kooijmans]

Hi Simone,

That was a quick fix! Unfortunately, it is still not correct. I added another case:

Assert.AreEqual(100, Price(0, 1, 1, 2, 2, 2, 3, 3, 3, 3, 4, 4, 4, 4, 4));

The test fails. It turns out the application comes with this solution: [0,1,2,3,4], [1,2,3,4], [2,3,4], [3,4], [4] . The price for this solution is 100.4

The optimal solution is [4], [3, 4], [0, 2, 3, 4], [1, 2, 3, 4], [1, 2, 3, 4] which has a price of 100.0.

I know this Potter Kata is tyipcally used by people to practice TDD. My problem with TDD is: when do you stop adding more test cases? When are you confident that your application is correct?

Regards,

Sander

[Simone Busoli]

Hi Sander,

that's a good point. As I mentioned (years) earlier in this thread mine was a quick solution initially driven by tests, until I quite soon thought about an algorithm to solve the problem. At some point you will have to stop and think about the logic behind it rather than just making small adjustments just to make tests pass. In my opinion at that point TDD doesn't help anymore until you find and start coding your new solution, and only then TDD starts helping again with the old tests you've written already and with the new ones you'll come up with. This as long as changing the implementation doesn't make your tests useless or even worse, prevents you from reusing them as they may no longer compile successfully. 

To answer your question about when to stop writing tests instead, I would say until you are confident you found a solution which doesn't leave space for any edge cases that you didn't cover. As you have discovered mine didn't by any means have these characteristics, although I gave another stab at it and I swear you won't be able to break this one :)

[Aitor Alzola]

Hi Sander 

 My problem with TDD is: when do you stop adding more test cases? When are you confident that your application is correct?

You have to think about this very same question with a non-TDD approach, don't you? The only difference is that you have to remember all the cases forever to test them again when you change something... I think it's a difficult question to answer in every software development scenarios :)

--
Aitor Alzola

[Sander Kooijmans]

Hi Simone,

Congratulations! Indeed I am unable to come up with another example that breaks your solution.

The funny thing is that your solution looks completely different from mine. I will post my solution in a couple of days on my website. When I do, I will post a link to my solution here.

Thanks for your time.

[Simone Busoli]

Very good, much relieved now :) Joking aside, I'm still not sure I came up with an optimal solution. I frankly didn't think enough about the mathematical, though simple, background that I feel this problem has, especially considering the last change I had to do to make it work correctly. In any case, looking forward to seeing yours.

Simone

[Sander Kooijmans]

Hi Simone,

I posted my solution on my website: http://gogognome.nl/2013/01/potter-kata-solution/

I looked at your solution. I have not tried to prove formally that your solution is correct. I suspect that it is only correct for the given discount percentages. The percentages were chosen such that dividing 8 books in 4 and 4 is better than 5 and 3. If the percentages are chosen such that 8 books should be divided in 2, 2, 2 and 2, then I fear your solution will fail. I have not tried this, so this is an interesting experiment I can try another day.

[Simone Busoli]

Hi Sander,

I appreciate that your solution is based on mathematical foundations that mine (or the others you mention in your blog) cannot praise. I would suggest that you don't take the correctness of (or lack thereof) other solutions too seriously though, strictly speaking for myself I don't recall having ever spent the time to come up with a solution which is formally correct, let alone correct under different assumptions (discount rates, that is) than those originally in the exercise. 

On the other hand I concur that for production code the naive approach is usually not advisable as without being able to prove the correctness of the solution, and especially when it comes to problems which require an underlying non-straightforward algorithm to be solved properly, you leave the door open to edge cases that you just couldn't think of, as you showed here. From experience though (which varies very much according to the field you work in), most problems either have a trivial solution or the optimal solution is too expensive to implement, which we approximate with implementations which are "good enough".

Overall, thanks for the valuable feedback.

Cheers, Simone

--
You received this message because you are subscribed to the Google Groups "software_craftsmanship" group.

To unsubscribe from this group and stop receiving emails from it, send an email to software_craftsma...@googlegroups.com.

To post to this group, send email to software_cr...@googlegroups.com.