SellOneItemTest.java

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

Attempt so far:

Introduced Catalogue class to move implementation detail from the Sale class.
Initially added Product class to reduce test duplication, but found it useful in the Catalogue class.
Added Empty product to catalogue by default as it allows removal of logic from the onBarcode method in the Sale class.
Test helper methods allow reduction of duplication in the tests.

Haven't added tests/logic to deal with "$0.00" -> "FREE", yet.

This comment is for revisions up to: https://gist.github.com/728975/8b64e4ebd4b442fad670426577c717070076107f

Looking good. I like the way you name product_1 and product_2 in your tests. My solution suffers without this ("magic numbers").

I thought about extracting Product, but didn't feel enough of a force pushing me in that direction. What tipped the balance for you?

Product sort of fell out looking purely at the tests, and my initial thought was to use in the tests themselves to remove duplication (in the form of "123", "456", etc being used in multiple places).

After having the product class I noticed I could get rid of the conditional logic in the onBarcode method, by introducing the Empty product, which makes that method concise.

I have a few questions.

1. Why did you choose to instantiate Catalogue, then provide it data in a separate step?

   catalogue = new Catalogue();
   addSampleProductsToCatalogue();
   

2. I like the idea of extracting the method assertSellingProductWithBarcodeGivesPrice(String barcode, String price), which I don't recall anyone else doing before.

3. I find this line of code a bit strange:

   assertSellingProductWithBarcodeGivesPrice("999", "No product with barcode 999");
   

The method name has the phrase "gives price", but I don't interpret "No product with barcode 999" as a price. This makes the method name somewhat inaccurate. In this situation, I try to make the method name more accurate and precise, as that generally uncovers a design problem. Try renaming the method to more accurately and precisely reflect how you use it here.

1. Similar to the preceding point, you model "Scanning error: empty barcode" as a price, and I don't think one could reasonably call this bit of text a price. Make the name more accurate and precise to uncover the underlying design problem.

In general, I see that you've used a sentinel value (Product.EMPTY) as an implementation shortcut. I can't quite articulate why I generally don't like sentinel values, even though I used them in years past in other languages. I think that, in this case, it has to do with not even needing to look in the Catalogue when someone scans an empty barcode.

Thanks for the feedback, I've taken what you've said on board and made several changes.

1. I removed the test setup helper. You were right, it added nothing. Although I feel now however that the setup is too large. Maybe it should be split up somehow? Or
2. I've added a new test helper with a more appropriate name. Since it calls the same code underneath, I delegate to the existing test helper to remove code duplication. Is that an acceptable approach to take?
3. I've removed the sentinel value and placed string messages in where I feel are the most relevant place to put them. Before implementing this, however, I added a DisplayRules class because it didn't seem like the responsibility of any of the existing classes to show "FREE". Hopefully now all the string return values are in the class that is most responsible for them. I also had to add the tests for the new class into the same test case since we're dealing with one file.

After spying on the comments you left for Louis Rose, I also added a test for null barcode.

Extracting private helper methods in the Domain classes.
Renaming test helpers to better reflect what the tests mean.

In general, I see many improvements. I like the short methods. Still, I have more comments.

1. 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?
2. The tests productSoldWithPriceOfZeroDisplayedAsFree and displayRulesShowFreeWhenPriceIsZero appear to duplicate their intent. Try to remove this duplication.
3. The tests displayRulesShowPriceWhenPriceIsNotZero, anotherProductFound and productFound appear to duplicate their intent. Try to remove this duplication.
4. The method Sale.invalid() refers only to its parameter, and not to the Sale instance. What could you conclude from this?
5. The method Sale.priceFor() refers only to the catalogue field, and not to the Sale instance. What could you conclude from this?
6. The method Sale.display() refers only to the display field, and not to the Sale instance. What could you conclude from this?
7. 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.