Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bookstore : add to track #310

Merged
merged 22 commits into from
Mar 17, 2017
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions config.json
Original file line number Diff line number Diff line change
Expand Up @@ -308,6 +308,11 @@
"slug": "list-ops",
"difficulty": 1,
"topics": []
},
{
"slug": "book-store",
"difficulty": 1,
"topics": []
}
],
"deprecated": [
Expand Down
17 changes: 17 additions & 0 deletions exercises/book-store/build.gradle
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
apply plugin: "java"
apply plugin: "eclipse"
apply plugin: "idea"

repositories {
mavenCentral()
}

dependencies {
testCompile "junit:junit:4.12"
}
test {
testLogging {
exceptionFormat = 'full'
events = ["passed", "failed", "skipped"]
}
}
Empty file.
62 changes: 62 additions & 0 deletions exercises/book-store/src/example/java/BookStore.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
import java.util.ArrayList;
import java.util.stream.Collectors;

public class BookStore{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

public static double CalculateTotalCost(ArrayList<Integer> books ){
return CalculateTotalCost(books,0);
}

public static double CalculateTotalCost (ArrayList<Integer> books,double priceSoFar ){
double minPrice = Double.MAX_VALUE;

if(books.size() == 0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

return priceSoFar;

ArrayList<Integer> groups = (ArrayList<Integer>) books.stream().distinct().collect(Collectors.toList());


double price = 0;

for(int i = 0;i<groups.size();i++){
books.remove(groups.get(i));
}


try {
price = CalculateTotalCost(books,priceSoFar + CostPerGroup(groups.size()));
} catch (Exception e) {
e.printStackTrace();
}
minPrice = Math.min(minPrice, price);
return minPrice;


}

private static double CostPerGroup(int groupSize) throws Exception{
double discountPercentage;
switch (groupSize)
{
case 1:
discountPercentage = 0;
break;
case 2:
discountPercentage = 5;
break;
case 3:
discountPercentage = 10;
break;
case 4:
discountPercentage = 20;
break;
case 5:
discountPercentage = 25;
break;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Contributor Author

@Twisti Twisti Mar 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Contributor

@matthewmorgan matthewmorgan Mar 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or even simpler. Perhaps take it in two steps:

  1. Check for invalid group size and throw and exception if invalid
  2. 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;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
}

default:
throw new Exception("Invalide group size : " + groupSize );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

return 8 * groupSize * (100 - discountPercentage) / 100;
}

}
Empty file.
Empty file.
105 changes: 105 additions & 0 deletions exercises/book-store/src/test/java/BookStoreTest.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
import static org.junit.Assert.assertEquals;

import java.util.ArrayList;
import java.util.Arrays;
import org.junit.Test;

public class BookStoreTest{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BookstoreTest please.


@Test
public void Basket_with_single_book(){
Integer[] p = {1};
ArrayList<Integer> books = new ArrayList<>(Arrays.asList(p));
Copy link
Contributor

@matthewmorgan matthewmorgan Mar 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

assertEquals(8,BookStore.CalculateTotalCost(books),2);
}


@Test
public void Basket_with_two_of_same_book(){
Integer[] p = {1,1};
ArrayList<Integer> books = new ArrayList<>(Arrays.asList(p));
assertEquals(16,BookStore.CalculateTotalCost(books),2);
}


@Test
public void Empty_basket(){
ArrayList<Integer> books = new ArrayList<>();
assertEquals(0,BookStore.CalculateTotalCost(books),2);
}


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

assertEquals(15.20,BookStore.CalculateTotalCost(books),2);
}


@Test
public void Basket_with_three_different_books(){
Integer[] p = {1,2,3};
ArrayList<Integer> books = new ArrayList<>(Arrays.asList(p));
assertEquals(21.6,BookStore.CalculateTotalCost(books),2);
}


@Test
public void Basket_with_four_different_books(){
Integer[] p = {1,2,3,4};
ArrayList<Integer> books = new ArrayList<>(Arrays.asList(p));
assertEquals(25.6,BookStore.CalculateTotalCost(books),2);

}


@Test
public void Basket_with_five_different_books(){
Integer[] p = {1,2,3,4,5};
ArrayList<Integer> books = new ArrayList<>(Arrays.asList(p));
assertEquals(30,BookStore.CalculateTotalCost(books),2);
}


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
         }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Test
public void Basket_with_eight_books(){
Integer[] p = {1,1,2,2,3,3,4,5};
ArrayList<Integer> books = new ArrayList<>(Arrays.asList(p));
assertEquals(51.20,BookStore.CalculateTotalCost(books),2);
}



@Test
public void Basket_with_nine_books(){
Integer[] p = {1,1,2,2,3,3,4,4,5};
ArrayList<Integer> books = new ArrayList<>(Arrays.asList(p));
assertEquals(55.60,BookStore.CalculateTotalCost(books),2);
}

@Test
public void Basket_with_ten_books(){
Integer[] p = {1,1,2,2,3,3,4,4,5,5};
ArrayList<Integer> books = new ArrayList<>(Arrays.asList(p));
assertEquals(60,BookStore.CalculateTotalCost(books),2);
}


@Test
public void Basket_with_eleven_books(){
Integer[] p = {1,1,2,2,3,3,4,4,5,5,1};
ArrayList<Integer> books = new ArrayList<>(Arrays.asList(p));
assertEquals(68,BookStore.CalculateTotalCost(books),2);
}


@Test
public void Basket_with_twelve_books(){
Integer[] p = {1,1,2,2,3,3,4,4,5,5,1,2};
ArrayList<Integer> books = new ArrayList<>(Arrays.asList(p));
assertEquals(75.20,BookStore.CalculateTotalCost(books),2);
}
}

1 change: 1 addition & 0 deletions exercises/settings.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ include 'binary'
include 'binary-search'
include 'binary-search-tree'
include 'bob'
include 'book-store'
include 'bracket-push'
include 'change'
include 'clock'
Expand Down