Skip to content

Instantly share code, notes, and snippets.

@louismrose
Forked from jbrains/SellOneItemTest.java
Created December 5, 2010 15:22
Show Gist options
  • Save louismrose/729166 to your computer and use it in GitHub Desktop.
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();
}
}
@louismrose
Copy link
Author

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.

@jbrains
Copy link

jbrains commented Dec 27, 2010

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.

@louismrose
Copy link
Author

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.

@jbrains
Copy link

jbrains commented Jan 19, 2011

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment