-
-
Save jbrains/731349 to your computer and use it in GitHub Desktop.
| ackage ca.jbrains.pos.test; | |
| import org.junit.*; | |
| import static org.junit.Assert.*; | |
| import java.util.*; | |
| public class SellOneItemTest { | |
| private POSDisplay posDisplay = new POSDisplay(); | |
| private Sale sale; | |
| public interface Catalog { | |
| Price lookupPrice(String barcode); | |
| boolean hasBarcode(String barcode); | |
| } | |
| public static class InMemoryCatalog implements Catalog { | |
| private Map<String, Price> pricesByBarcode; | |
| public InMemoryCatalog(Map<String, Price> pricesByBarcode) { | |
| if (pricesByBarcode == null) { | |
| throw new IllegalArgumentException("pricesByBarcode = " + pricesByBarcode); | |
| } | |
| this.pricesByBarcode = pricesByBarcode; | |
| } | |
| public Price lookupPrice(String barcode) { | |
| return pricesByBarcode.get(barcode); | |
| } | |
| public boolean hasBarcode(String barcode) { | |
| return pricesByBarcode.containsKey(barcode); | |
| } | |
| } | |
| public static class Sale { | |
| private POSDisplay posDisplay; | |
| private Catalog catalog; | |
| public Sale(POSDisplay posDisplay, Catalog catalog) { | |
| this.posDisplay = posDisplay; | |
| this.catalog = catalog; | |
| } | |
| public void onBarcode(String barcode) { | |
| if ("".equals(barcode)) { | |
| posDisplay.displayScannedEmptyBarcodeMessage(); | |
| return; | |
| } | |
| if (!catalog.hasBarcode(barcode)) { | |
| posDisplay.displayProductNotFoundMessage(barcode); | |
| return; | |
| } | |
| posDisplay.displayPrice(catalog.lookupPrice(barcode)); | |
| } | |
| } | |
| public static class Price { | |
| private int cents; | |
| public Price(int cents) { | |
| this.cents = cents; | |
| } | |
| public static Price inCents(int cents) { | |
| return new Price(cents); | |
| } | |
| public String format() { | |
| return cents == 0 ? "FREE" : NumberFormat.getCurrencyInstance().format(cents / 100.0d); | |
| } | |
| } | |
| public static class POSDisplay { | |
| private String text; | |
| private void setText(String text) { | |
| this.text = text; | |
| } | |
| public String getText() { | |
| return text; | |
| } | |
| public void displayScannedEmptyBarcodeMessage() { | |
| setText("Scanning error: empty barcode"); | |
| } | |
| public void displayProductNotFoundMessage(String barcode) { | |
| setText("No product with barcode " + barcode); | |
| } | |
| public void displayPrice(Price price) { | |
| setText(price.format()); | |
| } | |
| } | |
| @SuppressWarnings("serial") | |
| @Before | |
| public void setUp() { | |
| sale = new Sale(posDisplay, new InMemoryCatalog(new HashMap<String, Price>() { | |
| { | |
| put("123", Price.inCents(1250)); | |
| put("456", Price.inCents(2000)); | |
| put("333", Price.inCents(0)); | |
| } | |
| })); | |
| } | |
| @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 { | |
| assertEquals("FREE", Price.inCents(0).format()); | |
| } | |
| @Test(expected=RuntimeException.class) | |
| public void nullProductListIsInvalid() { | |
| new InMemoryCatalog(null); | |
| } | |
| private void assertForBarcodeDisplayShows(String barcode, String message) { | |
| sale.onBarcode(barcode); | |
| assertEquals(message, posDisplay.getText()); | |
| } | |
| } |
I could go further with this, but I hope the step-by-step changes give you some idea what I mean.
This looks great. I also went the route of extracting Price, but sadly did not come up with something like inCents() which I really enjoyed. Extracting the Catalog interface is also a big step towards reducing coupling.
A few questions:
- Could
POSDisplay.formatPriceThenDisplayIt()be renamed todisplayPrice()? - Are you concerned that
Price's dependency uponNumberFormatis opaque? Should it be injected? - Considering the extreme simplicity of the problem, would you normally go this far in designing? Are there multiple thresholds of problem complexity that prompt you to design/decouple more and more?
- Do we end it here? Any more notes/requirements?
- Absolutely, and I've renamed it.
- I'm not yet worried about it, because I think I understand
NumberFormat.getCurrencyInstance().format()well enough to depend directly on it. At the slightest concern, I would inject aPriceFormatdependency that usesNumberFormat. I would do this especially if I found I wanted to use part ofNumberFormatthat I didn't know well. - In general, I can't describe how far I would go in decoupling a design without the context of adding a new feature or fixing a mistake. I haven't yet articulated the reasoning behind my choice to go further or not. I tend to go further than most, though. See http://bit.ly/dImBpJ for more.
- We can end whenever you'd like to end. If you'd like to do more, then let's do more. Just let me know and I have more features to add.
Great, let's add some more features! :)
Hello,
I have few questions:
- Why did you choose name Sale for class that acts as a controller ?
- Why you are using two methods for displaying messages on the screen instead of one ?
For example you have:
displayScannedEmptyBarcodeMessage
displayProductNotFoundMessage
and a test case that looks like:
@test
public void emptyBarcode() throws Exception {
assertForBarcodeDisplayShows("", "Scanning error: empty barcode");
}
In that case you have the same message in your test and in your code which is a duplication. Instead you could use the common method setText and use a constant in the Sale object.
class Sale {
protected static final String SCANNING_ERROR = "Scanning error: empty barcode"
}
@test
public void emptyBarcode() throws Exception {
assertForBarcodeDisplayShows("", Sale.SCANNING_ERROR);
}
In that case the duplication is removed, but the POSDisplay now would contain only two methods getText and setText which means that POSDisplay acts more like a simple Display instead of POSDisplay.
@flaviusstef, let's move this to a github.com project to add features. Please start a project for yourself with whichever code base you want, and we'll use the Wiki there for features and all that. Email me when you have done that.
@mgenov, here are some answers.
- I chose
Salebefore I knew the class would become a controller. It seemed like a reasonable name for an object that represented the interaction of selling a single item. Typically, once we add features like selling multiple items, printing receipts, and so on, we extract the domain behavior into a smaller class calledSale. - So far we only have two messages, and while I notice the duplication in the method names
display*Message(), I typically don't introduce aMessageobject until the thirddisplay*Message()method, using the "Three strikes and you refactor" rule.
Regarding introducing the SCANNING_ERROR constant, I generally don't like to introduce constants that say the same thing as the values they replace. Instead, I'd rather remove the duplication by introducing a ScanningErrorMessage class.
Sounds reasonable. Thanks @jbrains
btw I like the idea for the new project. These testing ideas, advices, practice examples and conversations are awesome.
Created project: https://github.com/flaviusstef/POS-TDD-Exercise
Let's have some fun there. :)
I apologise for the indentation throughout. I was using vim without any special Java bindings or settings. I forgot about the whole tab/space issue until I had finished.