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

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.