bookstore : add to track by Twisti · Pull Request #310 · exercism/java

Twisti · 2017-02-17T13:04:07Z

I'll work on the bookstore exercise using C# version
https://github.com/exercism/x-common/tree/master/exercises/book-store

jtigger · 2017-02-22T15:16:27Z

Renamed to make clear the status of this PR. @Twisti, when you're ready for a review, please rename it back to bookstore : add to track or the like. Thanks!

update config.json

Twisti · 2017-03-03T15:37:19Z

Ready for a review :)

matthewmorgan · 2017-03-05T14:33:09Z

+			break;
+		case 5:
+			discountPercentage = 25;
+			break;


I see a pretty clear relationship between groupSize and discountPercentage. Might there be a more concise, but equally readable, way to capture this?

Also wondering, is this really configuration data rather than business logic?

I don't the the relationship between groupSize and discountPercentage, can you tell me more about this?

Also I didn't understand your second question, can you develop and explain it ?

I just checked the php exemple of the exercise and saw a different version about the discount percentage :

$discount = [0, 0, 0.05, 0.1, 0.2, 0.25]; $price = 8; return $price * $count * (1 - $discount[$count]);

So in Java, it will be :

double[] discountPercentage = {0, 0, 0.05, 0.1, 0.2, 0.25}; return 8 * groupSize * (1 - discountPercentage[groupSize]);

It's this kind of relationship you're talking about ?

Or even simpler. Perhaps take it in two steps:

Check for invalid group size and throw and exception if invalid

return for the discounted group price:

8 * (groupSize - 1) * 0.05d;

Even better, avoid the use of magic numbers. Name the book price and discount increment, EG DISCOUNT_INCREMENT, BOOK_PRICE, define them in the class, and use:

BOOK_PRICE * (groupSize - 1) * DISCOUNT_INCREMENT;

As far as my "configuration vs logic" comment, it's not hard to imagine that the price of a book or the discount increment would be a business rule of the bookstore. In a production application, you would want to separate setting those values from the logic. You might even imagine a case in which you would have a class that is responsible for calculating the discount that is separate from the one that applies it to the user's purchase.

In this example, I feel like calling these values out by defining them at the class level with clear names is probably enough.

8 * (groupSize - 1) * 0.05d;

Are you sure that this work ? Or maybe I'm missing something ?
My method looks like this :

private double CostPerGroup(int groupSize) throws Exception{ if(groupSize > maxGroupSize || groupSize <= 0 ) throw new Exception("Invalide group size : " + groupSize ); double discountForOneBook = bookPrice * (groupSize - 1) * discountIncrement; return (bookPrice * groupSize ) - (groupSize * discountForOneBook); }

But by running gradle for the test, I have 13 tests completed, 5 failed :

BookStoreTest > TwoCopiesOfEachBook FAILED java.lang.AssertionError: expected:<60.0> but was:<64.0> at org.junit.Assert.fail(Assert.java:88) at org.junit.Assert.failNotEquals(Assert.java:834) at org.junit.Assert.assertEquals(Assert.java:553) at org.junit.Assert.assertEquals(Assert.java:683) at BookStoreTest.TwoCopiesOfEachBook(BookStoreTest.java:106) BookStoreTest > TwoEachOfFirst4BooksAnd1CopyEachOfRest FAILED java.lang.AssertionError: expected:<55.6> but was:<59.2> at org.junit.Assert.fail(Assert.java:88) at org.junit.Assert.failNotEquals(Assert.java:834) at org.junit.Assert.assertEquals(Assert.java:553) at org.junit.Assert.assertEquals(Assert.java:683) at BookStoreTest.TwoEachOfFirst4BooksAnd1CopyEachOfRest(BookStoreTest.java:97) BookStoreTest > ThreeEachOFirst2BooksAnd2EachOfRemainingBooks FAILED java.lang.AssertionError: expected:<75.2> but was:<79.2> at org.junit.Assert.fail(Assert.java:88) at org.junit.Assert.failNotEquals(Assert.java:834) at org.junit.Assert.assertEquals(Assert.java:553) at org.junit.Assert.assertEquals(Assert.java:683) at BookStoreTest.ThreeEachOFirst2BooksAnd2EachOfRemainingBooks(BookStoreTest.java:124) BookStoreTest > ThreeCopiesOfFirstBookAnd2EachOfRemaining FAILED java.lang.AssertionError: expected:<68.0> but was:<72.0> at org.junit.Assert.fail(Assert.java:88) at org.junit.Assert.failNotEquals(Assert.java:834) at org.junit.Assert.assertEquals(Assert.java:553) at org.junit.Assert.assertEquals(Assert.java:683) at BookStoreTest.ThreeCopiesOfFirstBookAnd2EachOfRemaining(BookStoreTest.java:115) BookStoreTest > TwoGroupsOfFourIsCheaperThanGroupOfFivePlusGroupOfThree FAILED java.lang.AssertionError: expected:<51.2> but was:<53.6> at org.junit.Assert.fail(Assert.java:88) at org.junit.Assert.failNotEquals(Assert.java:834) at org.junit.Assert.assertEquals(Assert.java:553) at org.junit.Assert.assertEquals(Assert.java:683) at BookStoreTest.TwoGroupsOfFourIsCheaperThanGroupOfFivePlusGroupOfThree(BookStoreTest.java:79)

So, I prefer to upload this modifications and don't push now.
BookStore_Java_Twisti.tar.gz

Sorry, I made an error! I thought the discount tiers were linear by groupSize, and they are not. My proposal and the file you uploaded both have a problem because of that. However, a solution like the PHP one you mentioned would work nicely:

private static double[] DISCOUNT_TIERS = {0,5,10,20,25}; //declare at class level private double CostPerGroup(int groupSize) throws Exception{ if (groupSize < 1 || groupSize > 5){ throw new Exception("Invalid group size : " + groupSize ); } return bookPrice * groupSize * (100 - DISCOUNT_TIERS[groupSize-1])/100; }

matthewmorgan · 2017-03-05T14:38:33Z

+		assertEquals(30,BookStore.CalculateTotalCost(books),2);
+	}
+
+


Looks like from this point on, the test suite diverges from the canonical test suite for this exercise. Is that intentional?

All the remaining tests are extra, and these tests from the canonical spec are missing:

{ "description": "Two groups of four is cheaper than group of five plus group of three", "basket": [1,1,2,2,3,3,4,5], "targetgrouping": [[1,2,3,4],[1,2,3,5]], "expected": 51.20 }, { "description": "Group of four plus group of two is cheaper than two groups of three", "basket": [1,1,2,2,3,4], "targetgrouping": [[1,2,3,4],[1,2]], "expected": 40.8 }, { "description": "Two each of first 4 books and 1 copy each of rest", "basket": [1,1,2,2,3,3,4,4,5], "targetgrouping": [[1,2,3,4,5],[1,2,3,4]], "expected": 55.60 }, { "description": "Two copies of each book", "basket": [1,1,2,2,3,3,4,4,5,5], "targetgrouping": [[1,2,3,4,5],[1,2,3,4,5]], "expected": 60.00 }, { "description": "Three copies of first book and 2 each of remaining", "basket": [1,1,2,2,3,3,4,4,5,5,1], "targetgrouping": [[1,2,3,4,5],[1,2,3,4,5],[1]], "expected": 68.00 }, { "description": "Three each of first 2 books and 2 each of remaining books", "basket": [1,1,2,2,3,3,4,4,5,5,1,2], "targetgrouping": [[1,2,3,4,5],[1,2,3,4,5],[1,2]], "expected": 75.20 }

I followed the test from the C# version, so this test is just a translation from C# to Java.
I will update my tests to add tests you have linked.

Yes, I see that!

Thanks for adding the additional tests. Along the way, before we merge your changes, we'll need you to add the annotation @Ignore before all but the first test in the suite. That way users can focus on getting one test at a time to pass.

matthewmorgan · 2017-03-05T14:40:38Z

@Twisti thanks for doing this!

In the spirit of challenging each other to produce the best possible product, I have two questions for you. Please let me know your thoughts.

Also, we merged some older, pending PRs this morning and that created a merge conflict with your config.json. Would you please resolve that, or let me know if you need help doing so?

matthewmorgan · 2017-03-12T18:04:39Z

+public class BookStoreTest{
+
+	@Test
+	public void Only_a_single_book(){


Standard convention in Java is to use camelCase for method and variable names. Save the snake_case for Python!

matthewmorgan · 2017-03-12T18:07:33Z

+	@Test
+	public void Only_a_single_book(){
+		Integer[] p = {1};
+		ArrayList<Integer> books = new ArrayList<>(Arrays.asList(p));


Normally when we declare an object like this in Java, we prefer to declare using the interface. In this case:

List<Integer> books = new ArrayList<>(Arrays.asList(p));

+1 for using the diamond operator!

matthewmorgan · 2017-03-12T18:15:59Z

@Twisti thanks for your additional efforts on this one.

I have made some specific comments in response to your questions, and called out a few more things that should be updated to conform to Java 'best practices.'

Finally, I must draw your attention to a conversation that some of the maintainers had around another exercise. Specifically, it was about preferring the use of instance methods rather than class methods. If you are comfortable making this change in your PR, I think it would be good.

Please let me know if you would like help with any of these things, I'd be pleased to assist.

Twisti · 2017-03-16T12:50:01Z

As you advised for me to do, I updated my code :

using camelCase for the names of the tests methods
adding @ignore annotation to the tests
using List instead of ArrayList
using instance methods instead of class method
modify magic numbers

matthewmorgan

@Twisti this is really coming along nicely!

Please consider the feedback I have given you for the discount method, but know that it is optional.

The Java style changes would be appreciated, as they are the standard here on exercism/xjava.

I've made a few comments about capitalization that apply in several places, but I didn't think it necessary to mention them all. They will need to be addressed in both the example code and the test code please.

Please let me know if I can assist you in any way.

matthewmorgan · 2017-03-17T01:12:39Z

+		double minPrice = Double.MAX_VALUE;
+
+
+		if(books.size() == 0)


Stylistically we would prefer if you use braces, even for simple cases like this. EG:

if (books.size() ==0){ return priceSoFar; }

matthewmorgan · 2017-03-17T01:13:09Z

+		this.bookPrice = 8;
+	}	
+
+	public double CalculateTotalCost(){


Method names should be camelCase, but not CapitalWords in Java. Save the CapitalWords for class names.

matthewmorgan · 2017-03-17T01:13:24Z

+import java.util.List;
+import java.util.stream.Collectors;
+
+public class BookStore{


In English we would say Bookstore rather than BookStore. Also, the folder name should likewise be one word bookstore without the hypen.

matthewmorgan · 2017-03-17T01:13:40Z

+		if(books.size() == 0)
+			return priceSoFar;
+
+		List<Integer> groups = (ArrayList<Integer>) books.stream().distinct().collect(Collectors.toList());


matthewmorgan · 2017-03-17T01:14:59Z

+			discountPercentage = 25;
+			break;
+		default:
+			throw new Exception("Invalide group size : " + groupSize );


You'll want to change the spelling to Invalid here.

matthewmorgan · 2017-03-17T01:15:24Z

+
+import org.junit.Test;
+
+public class BookStoreTest{


BookstoreTest please.

matthewmorgan · 2017-03-17T01:15:45Z

+public class BookStoreTest{
+
+	@Test
+	public void OnlyASingleBook(){


The camelCase Java rule would apply here.

matthewmorgan · 2017-03-17T01:19:18Z

+	@Test
+	public void OnlyASingleBook(){
+		Integer[] p = {1};
+		List<Integer> books = new ArrayList<>(Arrays.asList(p));


You can just pass the integers to Arrays.asList(). No need to declare and assign beforehand. EG:

List<Integer> books = new ArrayList<>(Arrays.asList(1));

or

List<Integer> books = new ArrayList<>(Arrays.asList(1,2,3));

matthewmorgan · 2017-03-17T01:28:52Z

+			break;
+		case 5:
+			discountPercentage = 25;
+			break;


Sorry, I made an error! I thought the discount tiers were linear by groupSize, and they are not. My proposal and the file you uploaded both have a problem because of that. However, a solution like the PHP one you mentioned would work nicely:

private static double[] DISCOUNT_TIERS = {0,5,10,20,25}; //declare at class level private double CostPerGroup(int groupSize) throws Exception{ if (groupSize < 1 || groupSize > 5){ throw new Exception("Invalid group size : " + groupSize ); } return bookPrice * groupSize * (100 - DISCOUNT_TIERS[groupSize-1])/100; }

…camelCase for methods

matthewmorgan

@Twisti this is really looking terrific. There are two small changes you should make to adhere to Java conventions and track style.

Also we will need to resolve the merge conflict in config.json that was caused my merging another PR earlier. If you need help with this please let me know.

After that this will be ready to merge!

matthewmorgan · 2017-03-17T13:25:54Z

+	private int bookPrice, maxGroupSize;
+	private double discountIncrement;
+	private List<Integer> books;
+	private static double[] discount_tiers = {0,5,10,20,25};   	


Normally a constant would be ALL_CAPITALS like this. Most editors will recognize them as such and make them easier to spot in the code.

I added static to constants BOOK_PRICE, MAX_GROUP_SIZE.
Also, I deleted the discountIncrement because it's useless now.

matthewmorgan · 2017-03-17T13:26:25Z

+
+
+		if(books.size() == 0)	
+			return priceSoFar;


Curly braces here please.

… bookstore-add

matthewmorgan · 2017-03-17T17:47:31Z

Beautiful! Thank you so much 🥇

dibs : I will implement exercise bookstore

9b71eb0

jtigger changed the title ~~bookstore : add to track~~ bookstore : dibs on adding to track Feb 22, 2017

Ladent Thibault and others added 4 commits March 3, 2017 15:50

bookstore : add to track

de35a3f

bookstore : add to track

c61580a

update config.json

Merge branch 'master' into bookstore-add

6d15792

Merge branch 'master' into bookstore-add

ab34101

Twisti changed the title ~~bookstore : dibs on adding to track~~ bookstore : add to track Mar 3, 2017

matthewmorgan reviewed Mar 5, 2017

View reviewed changes

Twisti and others added 3 commits March 10, 2017 14:22

Update config.json : added orc-numbers

7a9a4b4

Merge branch 'master' into bookstore-add

19ca6d0

add missing test from the canonical spec and delete the remaning test

c8dd1ff

matthewmorgan reviewed Mar 12, 2017

View reviewed changes

Ladent Thibault added 6 commits March 16, 2017 11:33

using camelCase instead of snake_case for tests methods

812e86e

adding @ignore annotation

9252786

using List instead of ArrayList

8426842

using instance methods instead of class method

f26f9e7

delete useless stuff

28c89b0

modify magic numbers

1142bff

matthewmorgan reviewed Mar 17, 2017

View reviewed changes

Ladent Thibault added 3 commits March 17, 2017 12:17

update CostPerGroup method

a551793

update class name and files name to Bookstore from Bookstore + using …

c823390

…camelCase for methods

pass the integers directly to the Arrays.asList() methods

94e6ee0

matthewmorgan suggested changes Mar 17, 2017

View reviewed changes

Ladent Thibault and others added 5 commits March 17, 2017 14:56

adding brackets

18fd731

fix constant variable

b1f4e7f

Merge branch 'master' into bookstore-add

6e385d1

oMerge branch 'bookstore-add' of https://github.com/Twisti/xjava into…

84e3bb6

… bookstore-add

using static for constants

a8303ad

matthewmorgan approved these changes Mar 17, 2017

View reviewed changes

matthewmorgan merged commit 458f5c1 into exercism:master Mar 17, 2017

Uh oh!

Conversation

Twisti commented Feb 17, 2017

Uh oh!

jtigger commented Feb 22, 2017

Uh oh!

Twisti commented Mar 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Twisti Mar 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matthewmorgan Mar 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matthewmorgan commented Mar 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matthewmorgan Mar 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matthewmorgan commented Mar 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Twisti commented Mar 16, 2017

Uh oh!

matthewmorgan left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matthewmorgan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matthewmorgan commented Mar 17, 2017

Uh oh!

Reviewers

Assignees

Twisti Mar 10, 2017 •

edited

Loading

matthewmorgan Mar 12, 2017 •

edited

Loading

matthewmorgan Mar 12, 2017 •

edited

Loading

matthewmorgan commented Mar 12, 2017 •

edited

Loading

matthewmorgan left a comment •

edited

Loading