TDD Approach and Simple Solution

Question

How many tests could you write for the enum class below. I am looking for the following output given the following command:

java fileName HORIZON_BOX, HORIZON_BOX_WITH_CC, HORIZON_BOX_WITH_CC

1 HORIZON_BOX: 20.00
2 HORIZON_BOX_WITH_CC @ 30.00 : 60.00
GRAND TOTAL : 80.00

What is the best solution approach that you would go about solving this problem?

public enum Product {

    HORIZON_BOX(30.00),
    HORIZON_BOX_WITH_CC(50.00),
    HORIZON_BOX_WITH_CC_2_TB(100.00),
    HORIZON_MULTIROOM(75.00),
    HUB(20.00);

    private double price;

    private Product(double price) {
        this.price = price;
    }

    public double getPrice() {
        return price;
    }

}

The tests I have are as follows:

public class ProductTest {

/**
 * Test that fails as we cannot access the private constructor of the Enum
 * Written before any code is written to test
 */
@Test
public void testProductNothingImplemented() {
    // Product("Nothing has been implemented yet");

}

/**
 * Test expectedvalue of 20
 */
@Test
public void testProductHORIZONBoxHUB() {

    assertEquals(20, Product.HUB.getPrice(), 0);

}

/**
 * Test expectedvalue of 50 for HORIZON_BOX_WITH_HD
 */
@Test
public void testProductHORIZON_BOX_WITH_HD() {

    assertEquals(50, Product.HORIZON_BOX_WITH_CC.getPrice(), 0);

}

/**
 * Test expectedvalue of 100 for HORIZON_BOX_WITH_HD_2_TB
 */
@Test
public void testProductHORIZON_BOX_WITH_HD_2_TB() {

    assertEquals(100, Product.HORIZON_BOX_WITH_CC_2_TB.getPrice(), 0);

}

/**
 * Test expectedvalue of 75 for HORIZON_MULTIROOM
 */
@Test
public void testProductHORIZON_MULTIROOM() {

    assertEquals(75, Product.HORIZON_MULTIROOM.getPrice(), 0);

}

/**
 * Test expectedvalue of 30 for HORIZON_BOX
 */
@Test
public void testProductHORIZON_BOX() {

    assertEquals(30, Product.HORIZON_BOX.getPrice(), 0);

}

/**
 * test method to compute total with zero products
 */
@Test
public void testComputeEmptyMap() {
    Map<String, Integer> testMap = new HashMap<String, Integer>();

    //setup map
    Horizon Horizon = new Horizon();
    assertEquals(new Double(0), Horizon.computeAndPrintTotal(testMap));
}

/**
 * test method to compute total with two elements but second element is ) products
 */
@Test
public void testComputeAndPrintTotalWithOneProductHavingZeroFrequency() {
    Map<String, Integer> testMap = new HashMap<String, Integer>();

    //setup map
    testMap.put("HUB", 1);
    testMap.put("HORIZON_BOX", 0);

    Horizon Horizon = new Horizon();
    assertEquals(new Double(20), Horizon.computeAndPrintTotal(testMap));
}


/**
 * test method to compute total with two products
 */
@Test
public void testComputeAndPrintTotalPass() {
    Map<String, Integer> testMap = new HashMap<String, Integer>();

    testMap.put("HUB", 1);
    testMap.put("HORIZON_BOX", 2);

    Horizon Horizon = new Horizon();
    assertEquals(new Double(80), Horizon.computeAndPrintTotal(testMap));
}




}

#######################################################################################

What I have is the following: The Main class that executes is

public class Horizon {

private Map<String, Integer> argFrequencyCount = new HashMap<String, Integer>();

double total = 0;

@SuppressWarnings("rawtypes")
public static void main(String[] args) {

    Horizon horizon = new Horizon();

    for (String productName : args) {

        Integer frequency = horizon.argFrequencyCount.get(productName);

        horizon.argFrequencyCount.put(productName, (frequency == null) ? 1
                : frequency + 1);

    }

    horizon.argFrequencyCount = horizon.sortProductMapByFrequency(horizon.argFrequencyCount);

    for (Iterator iterator = horizon.argFrequencyCount.entrySet().iterator(); iterator
            .hasNext();) {
        @SuppressWarnings("unchecked")
        Map.Entry<String, Integer> productCount = (Map.Entry<String, Integer>) iterator
                .next();
        horizon.outputPrintHelper(productCount.getKey(),
                productCount.getValue());

    }
    horizon.computeAndPrintTotal(horizon.argFrequencyCount);

}

/**
 * method to print total
 * 
 * @param argFrequencyCount
 */
public Double computeAndPrintTotal(Map<String, Integer> argFrequencyCount) {

    for (Iterator<Map.Entry<String, Integer>> iterator = argFrequencyCount.entrySet().iterator(); iterator
            .hasNext();) {
        Map.Entry<String, Integer> productCount = (Map.Entry<String, Integer>) iterator
                .next();

        total = total + productCount.getValue()
                * Product.valueOf(productCount.getKey()).getPrice();

    }
    System.out.println("GRAND TOTAL : £" + total);
    return total;

}

/**
 * Method to print the items and corresponding item total
 * 
 * @param prod
 * @param numberOfItems
 */

private void outputPrintHelper(String prod, int numberOfItems) {

    Product product = Product.valueOf(prod);

    switch (product) {
    case HUB:
        if (numberOfItems == 1) {
            System.out.println(numberOfItems + " HUB : £"
                    + Product.HUB.getPrice());

        } else {
            System.out.println(numberOfItems + " HUB @  £" + Product.HUB
                    + " : £" + numberOfItems * Product.HUB.getPrice());
        }
        break;
    case HORIZON_BOX:
        if (numberOfItems == 1) {
            System.out.println(numberOfItems + " HORIZON_BOX : £"
                    + Product.HORIZON_BOX.getPrice());

        } else {
            System.out.println(numberOfItems + " HORIZON_BOX @  £"
                    + Product.HORIZON_BOX.getPrice() + " : £" + numberOfItems
                    * Product.HORIZON_BOX.getPrice());
        }
        break;
    case HORIZON_BOX_WITH_CC:
        if (numberOfItems == 1) {
            System.out.println(numberOfItems + " HORIZON_BOX_WITH_CC : £"
                    + Product.HORIZON_BOX_WITH_CC.getPrice());

        } else {
            System.out.println(numberOfItems + " HORIZON_BOX_WITH_CC @  £"
                    + Product.HORIZON_BOX_WITH_CC.getPrice() + " : £"
                    + numberOfItems * Product.HORIZON_BOX_WITH_CC.getPrice());
        }

        break;
    case HORIZON_BOX_WITH_CC_2_TB:
        if (numberOfItems == 1) {
            System.out.println(numberOfItems + " HORIZON_BOX_WITH_CC_2_TB : £"
                    + Product.HORIZON_BOX_WITH_CC_2_TB.getPrice());

        } else {
            System.out.println(numberOfItems
                    + " HORIZON_BOX_WITH_CC_2_TB @  £"
                    + Product.HORIZON_BOX_WITH_CC_2_TB.getPrice() + " : £"
                    + numberOfItems
                    * Product.HORIZON_BOX_WITH_CC_2_TB.getPrice());
        }

        break;
    case HORIZON_MULTIROOM:
        if (numberOfItems == 1) {
            System.out.println(numberOfItems + " HORIZON_MULTIROOM : £"
                    + Product.HORIZON_MULTIROOM.getPrice());

        } else {
            System.out.println(numberOfItems + " HORIZON_MULTIROOM @  £"
                    + Product.HORIZON_MULTIROOM.getPrice() + " : £"
                    + numberOfItems * Product.HORIZON_MULTIROOM.getPrice());
        }
    }

}

/**
 * Sort the HashMap by number of items in descending order
 * 
 */
private Map<String, Integer> sortProductMapByFrequency(
        Map<String, Integer> passedMap) {

    List<String> mapKeys = new ArrayList<String>(passedMap.keySet());

    List<Integer> mapValues = new ArrayList<Integer>(passedMap.values());

    Collections.sort(mapValues);
    Collections.sort(mapKeys);

    LinkedHashMap<String, Integer> sortedMap = new LinkedHashMap<String, Integer>();

    Iterator<Integer> valueIt = mapValues.iterator();
    while (valueIt.hasNext()) {
        Object val = valueIt.next();
        Iterator<String> keyIt = mapKeys.iterator();

        while (keyIt.hasNext()) {
            Object key = keyIt.next();
            String comp1 = passedMap.get(key).toString();
            String comp2 = val.toString();

            if (comp1.equals(comp2)) {
                passedMap.remove(key);
                mapKeys.remove(key);
                sortedMap.put((String) key, (Integer) val);
                break;
            }

        }

    }
    return sortedMap;

}

}

Answer 1

Product.getPrice is too trivial to warrant a test on its own, and testing the price of each product alone only tests your typing ability. If the test passes, it only means you typed the same price in the code and the test; it doesn't prove that the price is correct since you could have the same typo in both places.

Test code should have very little logic. As tests become more complicated, they will need to be tested themselves. This invites errors. But already you can see problems with the design of Horizon in the tests: the class should maintain the map of product frequencies itself--not in the test code or in Horizon.main.

You're essentially building a shopping cart for the Horizon program. It requires a simple API given the code you have.

public class Cart {
    void add(Product item);
    Set<Product> getItems();
    int getCount(Product item);
    double getTotal();
}

Here are some tests I would start with:

Cart fixture = new Cart();

@Test
void startsEmpty() {
    assertThat(fixture.getItems(), empty());
}

@Test
void oneItem() {
    fixture.add(Product.HORIZON_BOX);
    assertThat(fixture.getItems(), contains(Product.HORIZON_BOX));
    assertThat(fixture.getCount(Product.HORIZON_BOX), is(1));
    assertThat(fixture.getTotal(), is(30.0));
}

@Test
void combinesSameItems() {
    fixture.add(Product.HORIZON_BOX);
    fixture.add(Product.HORIZON_BOX);
    assertThat(fixture.getItems(), contains(Product.HORIZON_BOX));
    assertThat(fixture.getCount(Product.HORIZON_BOX), is(2));
    assertThat(fixture.getTotal(), is(60.0));
}

@Test
void doesNotCombineDifferentItems() {
    fixture.add(Product.HORIZON_BOX);
    fixture.add(Product.HORIZON_MULTIROOM);
    assertThat(fixture.getItems(), containsInAnyOrder(Product.HORIZON_BOX, HORIZON_MULTIROOM));
    assertThat(fixture.getCount(Product.HORIZON_BOX), is(1));
    assertThat(fixture.getCount(Product.HORIZON_MULTIROOM), is(1));
    assertThat(fixture.getTotal(), is(105.0));
}

Note that I'm using the Hamcrest assertions which are more expressive and easier to use and combine. The more recent versions of JUnit 4 (last couple years) are designed to use them as well. Also, when using @Test you don't need to start your test method names with test.

Answer 2

@David Harkness answered the question about Testing well enough, nothing important to add there. In the second question review of the design is asked, there are important issues with the design also, but I did not want the following points go unmade:

• You seem to be new to generics:

  @SuppressWarnings("rawtypes")
  

  If you had used Iterator<Map.Entry<String, Integer>> instead of a raw Iterator you would avoid not only this warning but also the unchecked cast (and the accompanying warning also)

• You should prefer for-each syntax when you iterate over a collection, unless you are adding or removing elements within the loop. Compare:

  for (Iterator iterator = horizon.argFrequencyCount.entrySet()
      .iterator(); iterator.hasNext();) {
      @SuppressWarnings("unchecked")
      Map.Entry<String, Integer> productCount = 
           (Map.Entry<String, Integer>) iterator.next();
  

  and this:

  for (Map.Entry<String, Integer> productCount : 
      horizon.argFrequencyCount.entrySet()) {
  

• Various Map<String, Integer> should be changed to Map<Product, Integer>; as these maps hold the "number of each product".

• You are losing type information here:

  Iterator<Integer> valueIt = mapValues.iterator();
  Object val = valueIt.next(); 
  
  
  Iterator<Integer> valueIt = mapValues.iterator();
  int val = valueIt.next(); 
  // you should use Integer val = valueIt.next(); 
  // if val could be null, not needed here
  

  For-each loops help here, too.

• And because you lost the type info, above; you are comparing the objects by comparing their string representations:

  String comp1 = passedMap.get(key).toString();
  String comp2 = val.toString();
  if (comp1.equals(comp2)) {
  

  Instead you could have written:

  if (passedMap.get(key).equals(val)) {
  

• Another point with this snippet is passedMap, key and val are not descriptive at all. Now compare:

   if (passedMap.get(key).equals(val)) {
  

  with:

   if (productFrequencies.get(product).equals(productFrequency)) {
  

  Which reads better? Names are important.

• Here is the signature of sortProductMapByFrequency:

  private Map<Product, Integer> sortProductMapByFrequency(
      Map<Product, Integer> passedMap) {
  

  The contract of the return value should be evident from the signature. If you are sorting a collection, the return value should have a definite order. So at least you should return a LinkedHashMap explicitly.

  If you are sorting the entries of a map according to the keys, you should return a SortedMap. Since you sort the entries depending on value, not just on key, you cannot. Then I would expect a List<Map.Entry<X,Y>>, if you are sorting the entries of a Map<X,Y>. [You cannot use SortedMap, straightforwardly, but you can use sorted multimaps, e.g. if you have Apache Commons or Guava. There is a maintainability trade-off there]

• Also when you iterate over a map and use both keys and value you should use entrySet to iterate, as you did elsewhere. Your nested loop could then be a single loop, more efficient more readable. Or you could do this:

  private List<Map.Entry<Product, Integer>> sortProductMapByFrequency(
          Map<Product, Integer> productFrequencies) {
  
      List<Entry<Product, Integer>> entries 
          = new ArrayList<Entry<Product, Integer>>(productFrequencies.entrySet());
  
      Collections.sort(entries, compareProductFrequencies());
  
      return entries;
  }
  
  private Comparator<Entry<Product, Integer>> compareProductFrequencies() {
      return new Comparator<Entry<Product, Integer>>() {
                  @Override
                  public int compare(Entry<Product, Integer> lhs,
                          Entry<Product, Integer> rhs) {
  
                      return lhs.getValue().compareTo(rhs.getValue());
                  }};
  }