SellOneItemTest.java

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 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.

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:

1. Could POSDisplay.formatPriceThenDisplayIt() be renamed to displayPrice()?
2. Are you concerned that Price's dependency upon NumberFormat is opaque? Should it be injected?
3. 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?
4. Do we end it here? Any more notes/requirements?

1. Absolutely, and I've renamed it.
2. 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 a PriceFormat dependency that uses NumberFormat. I would do this especially if I found I wanted to use part of NumberFormat that I didn't know well.
3. 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.
4. 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:

1. Why did you choose name Sale for class that acts as a controller ?
2. 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.

1. I chose Sale before 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 called Sale.
2. So far we only have two messages, and while I notice the duplication in the method names display*Message(), I typically don't introduce a Message object until the third display*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. :)