SellOneItemTest.java

import static org.junit.Assert.*;

import java.util.*;

import org.junit.*;


public class SellOneItemTest {
    public static class Sale {
		private Display display;
        private PriceList priceList;

		public Sale(Display display, Map<String, String> pricesByBarcode) {
            this.display = display;
            this.priceList = new PriceList(pricesByBarcode);
        }

        public void onBarcode(String barcode) {
            display.formatAndDisplay(textForBarcode(barcode));
        }

		private String textForBarcode(String barcode) {
			if (barcode.isEmpty())
                return "Scanning error: empty barcode";
            
            else if (priceList.doesNotInclude(barcode))
            	return "No product with barcode " + barcode;
			
			else
            	return priceList.priceFor(barcode);
		}
    }
    
    public static class PriceList {
		public Map<String, String> pricesByBarcode;

		public PriceList(Map<String, String> pricesByBarcode) {
			this.pricesByBarcode = pricesByBarcode;
		}

		public boolean doesNotInclude(String barcode) {
			return !pricesByBarcode.containsKey(barcode);
		}

		public String priceFor(String barcode) {
			return pricesByBarcode.get(barcode);
		}
	}

    public static class Display {
        private String text;

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

		private String format(String text) {
			if (isZeroDollars(text)) {
        		return "FREE";
        	} else {
        		return text;
        	}
		}

		private boolean isZeroDollars(String price) {
			return "$0.00".equals(price);
		}

        public String getText() {
            return text;
        }
    }
	
	private static Map<String, String> products() {
		return new HashMap<String, String>() {
            {
                put("123", "$12.50");
                put("456", "$20.00");
            }
        };
	}
    
    @Test
    public void productFound() throws Exception {
    	assertEquals("$12.50", textDisplayedFor("123"));
    }

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

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

    @Test
    public void emptyBarcode() throws Exception {
    	assertEquals("Scanning error: empty barcode", textDisplayedFor(""));
    }
    
    @Test
    public void noProductsToFind() throws Exception {
		assertEquals("Scanning error: empty barcode", textDisplayedFor("", null));
    }
    
    @Test
    public void productWithPriceZeroDisplayedAsFree() throws Exception {
    	Map<String, String> aFreeProduct = new HashMap<String, String>() {
    		{
    			put("789", "$0.00");
    		}
    	};

        assertEquals("FREE", textDisplayedFor("789", aFreeProduct));
    }
    
    @Test
    public void zeroDisplayedAsFree() throws Exception {
    	Display display = new Display();
    	display.formatAndDisplay("$0.00");
    	
    	assertEquals("FREE", display.getText());
    }
    
	private static String textDisplayedFor(String barcode) {
		return textDisplayedFor(barcode, products());
	}

	private static String textDisplayedFor(String barcode, Map<String, String> pricesByBarcode) {
		Display display = new Display();
		Sale sale = new Sale(display, pricesByBarcode);

		sale.onBarcode(barcode);

        return display.getText();
	}
}

I have a few questions.

1. I really like that you passed null for pricesByBarcode in nullBarcode(), although I can't tell the difference between what nullBarcode() checks and what emptyBarcode() checks.
2. I don't like the name of nullBarcode() because the barcode is "" and not null. Here, pricesByBarcode is null.
3. Naming the method textForBarcode() intrigues me, because I find the name structurally accurate and precise, and yet I don't remember seeing anyone else do that. Having named the method this way, I wonder why you kept the guard clause for the empty barcode out of this method. Why not move "Scanning error: empty barcode" into textForBarcode(), since it's also text for the empty barcode?
4. Why do you choose to look up a barcode in order to format the price $0.00? These seem separate responsibilities to me.
5. Why name the test method twoProducts()? I don't see any particular significance to the number of products you added to pricesByBarcode.
6. Why have the method twoProducts() return a HashMap? Do you rely on that specific class elsewhere in the code? I have the same question for textDisplayedFor(String, HashMap).

Thanks. Some thoughts in response:

1, 2. I agree. nullBarcode is a bad name for this test, and I now prefer noProductsToFind.
3. Thanks -- the duplication of the display.setText(...) statements pushed me to extract textForBarcode(). I didn't move "Scanning error: empty barcode" into textForBarcode(), because I wanted to treat empty and non-empty barcodes differently. On reflection, I agree that it is better to move "Scanning error: empty barcode" into textForBarcode().
4. I found this comment to be very insightful. I had not thought about the two responsibilities in textForBarcode. Extracting the calls to get and containsKey into their own methods led me to extract a class, Inventory.
5. You're right: twoProducts is a poor name. I now prefer products, which I'm not sure I like because it seems too general.
6. The parameters of type HashMap were created by Eclipse's extract method refactoring, presumably because I was using HashMap in the tests. The code uses Map now. It's interesting to see how my use of automated refactoring spread HashMap throughout the code.

I've learnt that I need to look harder to identify the responsibilities of a method / class, and that separating responsibilities can give rise to new abstractions.

I like introducing a class to represent the relationship between barcode and price, but I don't like the name Inventory. I put define:inventory into Google and got this:

Inventory is a list for goods and materials, or those goods and materials themselves, held available in stock by a business.

This agrees with my general understanding of the word "inventory": how many items of each product do we have in stock. The data structure relates a barcode to its price. Could you propose a more accurate name for this class?

I found this comment to be very insightful. I had not thought about the two responsibilities in textForBarcode.

Even so, the test to format $0.00 as FREE still looks up a barcode in order to check formatting the price correctly. Try writing a test for formatting $0.00 that doesn't involve looking up a barcode.

I've learnt that I need to look harder to identify the responsibilities of a method / class, and that separating responsibilities can give rise to new abstractions.

I find that additional tests help me find opportunities to separate responsibilities. Specifically, I look for duplication of irrelevant details in tests: this means duplicating parts of the test that don't directly affect the result I want to check. When I see duplication of this kind, it usually points to a new abstraction that wants to come out.

Thanks again!

Could you propose a more accurate name for this class?

I've chosen PriceList now. However, I don't like the potential for confusion between a physical list and the data structure. Do you have any other tips for choosing good names?

Even so, the test to format $0.00 as FREE still looks up a barcode in order to check formatting the price correctly. Try writing a test for formatting $0.00 that doesn't involve looking up a barcode.

Ok, I see another responsibility now. I've written the simplest test that I could imagine, zeroDisplayedAsFree(). I think the Display class arguably now has two responsibilities (storing a value and formatting prices) but I'm not sure I need to separate them just yet.

I find that additional tests help me find opportunities to separate responsibilities. Specifically, I look for duplication of irrelevant details in tests: this means duplicating parts of the test that don't directly affect the result I want to check. When I see duplication of this kind, it usually points to a new abstraction that wants to come out.

Thanks, that makes sense. In fact, the productWithPriceZeroDisplayedAsFree() test seems to be a good example of irrelevant detail. Like you said, the test requires the code to look up a product's price and format it, but I only want to test the latter.

I've chosen PriceList now. However, I don't like the potential for confusion between a physical list and the data structure. Do you have any other tips for choosing good names?

I understand the concern about including the word List in the name of the class. In this case, forget a software system for a moment: what would you call the physical thing that you'd consult to find the price of a product?

Ok, I see another responsibility now. I've written the simplest test that I could imagine, zeroDisplayedAsFree(). I think the Display class arguably now has two responsibilities (storing a value and formatting prices) but I'm not sure I need to separate them just yet.

I feel nervous about a method named setText() doing more than simply storing the value I pass in. The name just doesn't fit the purpose. In this situation, I try renaming the method to something more precise, then splitting the behavior based on the new name. In this case, I think you've gone most of the way.

Once more, thanks for the comments J. B.

I understand the concern about including the word List in the name of the class. In this case, forget a software system for a moment: what would you call the physical thing that you'd consult to find the price of a product?

I think, in the real world, a price list is an appropriate name for the thing that I'd use to find the price of a product. (It's a shame that there's no domain expert to ask here!)

I feel nervous about a method named setText() doing more than simply storing the value I pass in. The name just doesn't fit the purpose. In this situation, I try renaming the method to something more precise, then splitting the behavior based on the new name. In this case, I think you've gone most of the way.

Yes, you're right. setText() is a bad name now. I prefer formatAndDisplay in this version.

I think I'm done -- thanks for the guidance. I've been using some of the tips in my day-to-day work, and feel like my code is more expressive and easier to refactor as a result.

You're welcome, Louis. I'm glad I could help. Please write a little about your experience, and enjoy the practice. Take care.