-
-
Save louismrose/729166 to your computer and use it in GitHub Desktop.
| 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(); | |
| } | |
| } |
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.
I like introducing a class to represent the relationship between barcode and price, but I don't like the name
Inventory. I putdefine:inventoryinto Google and got this: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?
Even so, the test to format
$0.00asFREEstill looks up a barcode in order to check formatting the price correctly. Try writing a test for formatting$0.00that doesn't involve looking up a barcode.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.