SellOneItemTest.java

package ca.jbrains.pos.test;

import org.junit.*;
import static org.junit.Assert.*;
import java.util.*;

public class SellOneItemTest {

	private POSDisplay display = new POSDisplay();
	private Sale sale;

	public static class Sale {
		private POSDisplay display;
		private Map<String, String> pricesByBarcode = new HashMap<String, String> ();

		public Sale(POSDisplay d, Map<String, String> pricesByBarcode) {
			if (pricesByBarcode == null) {
				throw new RuntimeException("Invalid price list");
			}
			display = d;
			this.pricesByBarcode = pricesByBarcode;
		}

		public void onBarcode(String barcode) {
			if ("".equals(barcode)) {
				display.onEmptyBarcode();
				return;
			}

			if (!pricesByBarcode.containsKey(barcode)) {
				display.onInvalidBarcode(barcode);
				return;
			}
			display.showPrice(pricesByBarcode.get(barcode));
		}
	}

	public static class Display {
		private String text;

		public void setText(String text) {
			this.text = text;
		}

		public String getText() {
			return text;
		}
	}

	public static class POSDisplay extends Display {
		public void onEmptyBarcode() {
			setText("Scanning error: empty barcode");
		}

		public void onInvalidBarcode(String barcode) {
			setText("No product with barcode " + barcode);
		}

		public void showPrice(String price) {
			setText(price.equals("$0.00") ? "FREE" : price);
		}		
	}

	@SuppressWarnings("serial")
	@Before
	public void setUp() {
		sale = new Sale(display, new HashMap<String, String>() {
			{
				put("123", "$12.50");
				put("456", "$20.00");
				put("333", "$0.00");
			}
		});
	}

	@Test
	public void productFound() throws Exception {
		assertForBarcodeDisplayShows("123", "$12.50");
	}

	@Test
	public void anotherProductFound() throws Exception {
		assertForBarcodeDisplayShows("456", "$20.00");
	}

	@Test
	public void productNotFound() throws Exception {
		assertForBarcodeDisplayShows("999", "No product with barcode 999");
	}

	@Test
	public void emptyBarcode() throws Exception {
		assertForBarcodeDisplayShows("", "Scanning error: empty barcode");
	}

	@Test
	public void freeProductHasDistinctFormat() throws Exception {
		assertForBarcodeDisplayShows("333", "FREE");
	}

	@Test(expected=RuntimeException.class)
	public void nullProductListIsInvalid() {
		new Sale(display, null);
	}

	private void assertForBarcodeDisplayShows(String barcode, String message) {
		sale.onBarcode(barcode);
		assertEquals(message, display.getText());
	}
}

Steps taken:

1. introduced Catalogue abstraction
2. created setUp() to initialize SUT and DOCs
3. encapsulated the pricesByBarcode collection

Possible future abstractions: Product, Price.

Some questions.

1. Why setPricesByBarcode()? I don't think this extra complication in the design adds value to any production client.
2. Why _pricesByBarcode == null? Why allow a required field ever to be null?
3. I think I can delete removeBarcode() without failing any test, so why did you write it?
4. Why do I have to scan a barcode to check formatting $0 as "FREE"?
5. Why prefix fields with underscore?

Thank you for the feedback.

Refactored:

1. Forgotten it in, unnecessary, removed it.
2. Extraneous check, condition is never true. Removed it.
3. I was wrong. Added code without test. For symmetry.
4. Correct. The test fixture was too big. Minimized it.
5. It's a convention I defined for this problem. I researched the tubes, found out Sun discourages underscores apart from constant names. My idea is "_fieldname" is shorter than "this.fieldname". Perhaps you'd like to share the advantages of using "this." if that was the purpose of the question.

First, a response to your comment about this.field or _field. I typically simply use field except when I assign a new value to field, when I tend to write this.field = field because I tend to name the method parameter field as well.

Examples:

public Object(Collaborator collaborator) {
    this.collaborator = collaborator;
}

public void doSomething() {
    collaborator.doSomethingElse();
}

Next, thanks for playing along. I hope this helps you.

Now, some new questions.

1. You had added removeBarcode() "for symmetry". What about that symmetry did/do you value?
2. Why addProduct()? I don't think this extra complication in the design adds value to any production client.
3. Why removeProduct()? I don't think this extra complication in the design adds value to any production client.
4. Why do I have to find a $0-priced product in the Catalog to check formatting $0 as "FREE"?
5. Why did you write the test emptyCatalogueIsSearchable()? I don't see the value in it.
6. Why did you write the test itemsCanBeAddedAndRemovedFromCatalogue()? I don't see the value in it.
7. Why do you assign a value to the field _display twice - once in the field definition and once in the @Before method?

Thank you for bearing with me.
0 Since I don't professionally develop in Java, I take the word of a much wiser man and give up the underscore for field names.

1. removeBarcode() doesn't exist, I guess we are actually referring to removeProduct() - If you are implying that I did not need it as per the requirements of the problem/YAGNI rule, then you are right. But as I like interfaces to be symmetrical, Catalogue was blessed with both addProduct() and removeProduct(). Is symmetry in this case, in your oppinion, overrated ?
2. Who then, should encapsulate the list of products? Or do you consider the initial variant (Sale receiving a Map in the constructor) as being simpler?
3. see 2.
4. Are you implying the need for a Product class, whose responsibility it would be to map between "$0" and "FREE"? I view that mapping as being more of a marketing stunt, and as such the responsibility of Catalogue.
5. Added during development, as I was unsure of what Map.containsKey() does on an empty Map. Removed it.
6. That's the test code that covers addProduct() and removeProduct(). Have refactored it because it was testing the SUT through a DOC. I imagine your objection being linked to numbers 2. and 3. I haven't yet figured out the path you'd like to take me on, so a little bit more explicitness would be very welcomed.
7. My mistake. Removed the double.

Thank you for your willingness to think about my comments. I don't judge; I simply offer comments and ask people to reflect on them.

1. I did mean removeProduct(). You say you like interfaces to be symmetrical. I don't value symmetry on its own, except perhaps in an API I publish for third-party use. Especially for this first feature, I don't see the value of symmetry in this case.

2. I don't see the need to encapsulate the collection of Products yet. I do prefer simply letting the Sale take pricesByBarcode in its constructor. If you have any concerns about clients changing the contents of the Map without your knowledge, then consider storing either a copy or an unmodifiable view. (Java offers a way to mark a collection as unmodifiable.)

3. I don't mean that we need a Product class yet. I merely suggest that I shouldn't have to look up a product of price $0 in order to format the price as "FREE". I find it strange that a Catalogue would take responsibility for presenting a price as text. That sounds like a view-related responsibility.

4. When I don't know what an API method does, like Map.containsKey(), I write a Learning Test for it, like this:

   public class MapContainsKeyTest {
       @Test public void emptyMap() {
           assertFalse(new HashMap().containsKey("anything"));
        }
    }
   

5. I understand your impulse to test addProduct() and removeProduct() directly, but if pass the pricesByBarcode into the constructor for Sale, you can delete these methods and their corresponding tests.

Here's another go, starting from scratch.

1. Moved all presentation logic (string generation) to a Display son, POSDisplay. I am interested to see whether you consider this responsibility (special formatting of empty barcode, invalid barcode and free product) to be in the right place(POSDisplay), or instead as being Sale's responsibility.
2. Created assertForBarcodeDisplayShows(), stealing the idea from one of the other participants.
3. Enforced the necessity of a non-null pricesByBarcode() via an exception.

Side note: I have a nagging voice in my head telling me that, with the tests in their current situation, Display gets exercised only via Sale. Is the voice right? Should I be adding tests for POSDisplay?

First, my own observations, and then my comments on your comments.

1. Why the inconsistency between lines 20 and 21? If you plan to write this.x = x; in constructors, then I recommend doing it uniformly.
2. I'd like the exception you throw on a null pricesByBarcode to describe the problem more precisely.
3. You have decided to design Sale to require non-null a pricesByBarcode even though there is a code path through onBarcode() that doesn't require pricesByBarcode. If you sometimes need one and sometimes don't, what might that say about the design?
4. I see that you've moved methods responsible for displaying something into POSDisplay, which I quite like.
5. I wonder why you named the methods in POSDisplay inconsistently. You have the imperative showPrice() but the event handling onInvalidBarcode() and onEmptyBarcode().
6. You named the method onInvalidBarcode(), but is a barcode not in our catalog truly invalid? What other method name would describe this special case more accurately?
7. Why have Display and POSDisplay as separate classes?
8. I see that formatting $0 as "FREE" now appears in the view, which I find quite sensible. Now look at the method showPrice(String price). Should we model a price as text?

Otherwise, I like what you've done, and especially assertForBarcodeDisplayShows().

Regarding whether to write tests directly for POSDisplay, I think you can survive without it for a moment.

I decided to make a small number of improvements at https://gist.github.com/731349 to show you an example of how I approach the code you're provided here. You can clone the entire repository to step through the commits and examine the differences.