-
-
Save bishboria/728975 to your computer and use it in GitHub Desktop.
package ca.jbrains.pos.test; | |
import static org.junit.Assert.*; | |
import java.util.*; | |
import org.junit.*; | |
public class SellOneItemTest { | |
private Display display; | |
private DisplayFormatter displayFormatter; | |
private Catalogue catalogue; | |
private Sale sale; | |
private Product product_1, product_2, product_3; | |
@Before | |
public void setup() { | |
displayFormatter = new DisplayFormatter(); | |
display = new Display(displayFormatter); | |
catalogue = new Catalogue(); | |
product_1 = new Product("123", "$12.50"); | |
product_2 = new Product("456", "$20.00"); | |
product_3 = new Product("789", "$0.00"); | |
catalogue.addProduct(product_1); | |
catalogue.addProduct(product_2); | |
catalogue.addProduct(product_3); | |
sale = new Sale(display, catalogue); | |
} | |
public void afterScanningTheDisplayShows(String code, String message) { | |
sale.onBarcode(code); | |
assertEquals(message, display.displayFormattedText()); | |
} | |
@Test | |
public void productFound() throws Exception { | |
afterScanningTheDisplayShows(product_1.barcode(), product_1.price()); | |
} | |
@Test | |
public void anotherProductFound() throws Exception { | |
afterScanningTheDisplayShows(product_2.barcode(), product_2.price()); | |
} | |
@Test | |
public void productSoldWithPriceOfZeroDisplayedAsFree() throws Exception { | |
afterScanningTheDisplayShows(product_3.barcode(), "FREE"); | |
} | |
@Test | |
public void productNotFound() throws Exception { | |
afterScanningTheDisplayShows("999", "No product with barcode 999"); | |
} | |
@Test | |
public void emptyBarcode() throws Exception { | |
afterScanningTheDisplayShows("", "Scanning error: empty barcode"); | |
} | |
@Test | |
public void nullBarcode() throws Exception { | |
afterScanningTheDisplayShows(null, "Scanning error: empty barcode"); | |
} | |
} | |
class Sale { | |
private Display display; | |
private Catalogue catalogue; | |
public Sale(Display display, Catalogue catalogue) { | |
this.display = display; | |
this.catalogue = catalogue; | |
} | |
public void onBarcode(String barcode) { | |
String text; | |
if (invalid(barcode)) { | |
text = "Scanning error: empty barcode"; | |
} else { | |
text = catalogue.priceFor(barcode); | |
} | |
display.setText(text); | |
} | |
private boolean invalid(String barcode) { | |
return barcode == null || "".equals(barcode); | |
} | |
} | |
class Catalogue { | |
private Products products = new Products(); | |
public Catalogue() {} | |
public String priceFor(String barcode) { | |
if (products.productExistsFor(barcode)) | |
return products.priceForProductWith(barcode); | |
else | |
return "No product with barcode " + barcode; | |
} | |
public void addProduct(Product product) { | |
products.addProduct(product); | |
} | |
} | |
class Products { | |
private Map<String, Product> productsByBarcode; | |
public Products() { | |
productsByBarcode = new HashMap<String, Product>(); | |
} | |
String priceForProductWith(String barcode) { | |
return productsByBarcode.get(barcode).price(); | |
} | |
public boolean productExistsFor(String barcode) { | |
return productsByBarcode.containsKey(barcode); | |
} | |
public void addProduct(Product product) { | |
productsByBarcode.put(product.barcode(), product); | |
} | |
} | |
class Product { | |
private String barcode; | |
private String price; | |
public Product(String barcode, String price) { | |
this.barcode = barcode; | |
this.price = price; | |
} | |
public String barcode() { | |
return barcode; | |
} | |
public String price() { | |
return price; | |
} | |
} | |
class DisplayFormatter { | |
public String format(String text) { | |
if (shouldDisplayAsFree(text)) | |
return "FREE"; | |
else | |
return text; | |
} | |
private boolean shouldDisplayAsFree(String text) { | |
return "$0.00".equals(text); | |
} | |
} | |
class Display { | |
private String text; | |
private DisplayFormatter displayFormatter; | |
public Display(DisplayFormatter displayFormatter) { | |
this.displayFormatter = displayFormatter; | |
} | |
public void setText(String text) { | |
this.text = text; | |
} | |
public String displayFormattedText() { | |
return displayFormatter.format(text); | |
} | |
} |
In general, I see many improvements. I like the short methods. Still, I have more comments.
- I see that it takes you several statements to create the
Catalogue
in the state you want for the tests. Could you do this in one statement? What would that look like? - The tests
productSoldWithPriceOfZeroDisplayedAsFree
anddisplayRulesShowFreeWhenPriceIsZero
appear to duplicate their intent. Try to remove this duplication. - The tests
displayRulesShowPriceWhenPriceIsNotZero
,anotherProductFound
andproductFound
appear to duplicate their intent. Try to remove this duplication. - The method
Sale.invalid()
refers only to its parameter, and not to theSale
instance. What could you conclude from this? - The method
Sale.priceFor()
refers only to thecatalogue
field, and not to theSale
instance. What could you conclude from this? - The method
Sale.display()
refers only to thedisplay
field, and not to theSale
instance. What could you conclude from this? - The method
Display.getText()
appears to do more than its name implies. How would you rename this method more precisely? What might you conclude from the method's new name?
Thanks.
Given comments 4 to 6 I can see that I've got some feature envy. I will move those better named methods to the correct classes. Given that, it looks like there is still another class wanting to come out from the Catalgoue class.
Test duplication: My thoughts on this are that I should remove the tests I added to drive the DisplayRules class. I could also remove one of either productFound or anotherProductFound. Is it normal to remove tests that "essentially" duplicate behaviour once that logic has been driven out?
Set up of the Catalogue I'll need to think about some more. addProducts method perhaps... I'm not sure.
I had a horrible feeling about using DisplayRules inside Display originally. I'm thinking the text should be formatted by the DisplayRules class before being sent to the display. The fact I used the word formatted also suggests that I should rename DisplayRules to DisplayFormatter.
I'm also thinking that I should be testing the units more instead of doing, what is essentially, integration testing with the Sale class. This little problem has quite a few options! :)
I'll make these changes soon, this comment was part answering your questions, part me trying not to forget ideas...
Is it normal to remove tests that "essentially" duplicate behaviour once that logic has been driven out?
I find it common, yes. Regarding whether to remove the tests, I don't always act the same way. The Novice rule is "keep the tests", and when I first started practising test-first programming, I usually kept the tests, even when they were duplicates. Now I tend to remove the duplicate tests when I feel confident and retain them when I feel less confident.
Set up of the Catalogue I'll need to think about some more. addProducts method perhaps... I'm not sure.
What could you do to avoid addProducts()
or addProduct()
entirely?
I had a horrible feeling about using DisplayRules inside Display originally. I'm thinking the text should be formatted by the DisplayRules class before being sent to the display. The fact I used the word formatted also suggests that I should rename DisplayRules to DisplayFormatter.
That sounds reasonable, and I recommend you try it. This looks like a textbook case of inverting the dependency.
I'm also thinking that I should be testing the units more instead of doing, what is essentially, integration testing with the Sale class. This little problem has quite a few options! :)
Pick one and try it; that's the beauty of exercises like these.
Extracting private helper methods in the Domain classes.
Renaming test helpers to better reflect what the tests mean.