TDD and use case: cook dish with substitutions

Question

This is code for a class that takes available ingredients and returns left over ingredients after making a dish.

• Cesars: 2 carrot 4 ice burg 1 chicken 1 beans
• Russian: 2 carrots 2 beans 2 chicken

If carrot is not there - 1 bean or 2 iceberg can replace if beans is not there - 1 iceberg can replace if chicken is not there 2 carrots or 2 beans can replace.

GitHub has the full question.

package academic.cookSalad;

public enum Dish {
    CEASERS,RUSSIAN;
}


package academic.cookSalad;

public class Status {
    private Ingredients leftOverIngrediants;//Ingredients passed in minus what was used. Or same what was passed in case too few ingredients

    private   boolean success;//false if could not make else true

    public Ingredients getLeftOverIngrediants() {
        return leftOverIngrediants;
    }

    public void setLeftOverIngrediants(Ingredients leftOverIngrediants) {
        this.leftOverIngrediants = leftOverIngrediants;
    }

    public boolean isSuccess() {
        return success;
    }

    public void setSuccess(boolean success) {
        this.success = success;
    }

}

package academic.cookSalad;

public class Ingredients implements Cloneable {

    private int carrotQty;
    private int iceBergLettuceQty;

    private int chickenQty;

    private int beansQty;

    public int getCarrots() {
        return carrotQty;
    }

    public void setCarrots(int carrotQty) {
        this.carrotQty = carrotQty;
    }

    public int getIceBergLettuce() {
        return iceBergLettuceQty;
    }

    public void setIceBergLettuce(int iceBergLettuceQty) {
        this.iceBergLettuceQty = iceBergLettuceQty;
    }

    public int getChickens() {
        return chickenQty;
    }

    public void setChickens(int chickenQty) {
        this.chickenQty = chickenQty;
    }

    public int getBeans() {
        return beansQty;
    }

    public void setBeans(int beansQty) {
        this.beansQty = beansQty;
    }

    public Object clone() throws CloneNotSupportedException{        
        return super.clone();
    }

    public boolean equalValues(Ingredients other){
        return this.beansQty == other.beansQty &&
                this.carrotQty == other.carrotQty && this.chickenQty == other.chickenQty
                && this.iceBergLettuceQty == other.iceBergLettuceQty;
    }
}

package academic.cookSalad;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

public class Cook {
    private static final Logger logger = LoggerFactory.getLogger(Cook.class);
    Status cookIt(Ingredients in, Dish what) {
        logger.debug("v03");
        Status sta = new Status();
        sta.setLeftOverIngrediants(in);

        Ingredients curr = null;
        try {
            curr = (Ingredients) in.clone();
        } catch (CloneNotSupportedException e) {
            e.printStackTrace();
            logger.error("Clone did not work do a manual copy to curr ingredients TODO");
        }
        Ingredients req = new Ingredients();

        /*
         * Cesars: Russian: 2 carrot 2 carrots 4 ice burg 2 beans 1 chicken 2
         * chicken 1 beans
         */
        if (what == Dish.CEASERS) {
            req.setCarrots(2);
            req.setIceBergLettuce(4);
            req.setChickens(1);
            req.setBeans(1);
        } else {
            req.setCarrots(2);
            req.setBeans(2);
            req.setChickens(2);
            // req.setIceBergLettuceQty(0);//Not required
        }

        processCarrrots(curr, req, sta);
        if (sta.isSuccess()) {
            processBeans(curr, req, sta);
            if (sta.isSuccess()) {
                processChickens(curr, req, sta);
                if (sta.isSuccess()) {
                    processIceburg(curr, req, sta);
                    if (sta.isSuccess()) {
                        sta.setLeftOverIngrediants(curr);
                    }
                }

            }

        }

        return sta;
    }

    private void processChickens(Ingredients curr, Ingredients req, Status sta) {
        for (int i = 0; i < req.getChickens(); i++) {
            processOneChicken(curr, sta);
            if (!sta.isSuccess()) {
                if (curr.getBeans() > 1) {
                    // could have put a loop if more than 2
                    // don't need to check for success as we checked available
                    // quantity already
                    processOneBean(curr, sta);
                    processOneBean(curr, sta);
                } else if (curr.getCarrots() > 1) {
                    processOneCarrot(curr, sta);
                    processOneCarrot(curr, sta);
                } else {
                    sta.setSuccess(false);
                    return;
                }

            }
        }

    }

    private void processCarrrots(Ingredients curr, Ingredients req, Status sta) {
        for (int i = 0; i < req.getCarrots(); i++) {
            processOneCarrot(curr, sta);
            if (!sta.isSuccess()) {
                // check if failed after every ingredient. Else some other
                // ingredient might
                // be there and we get wrong result in some cases.
                // 1 bean or 2 ice
                if (curr.getBeans() > 0) {
                    processOneBean(curr, sta);
                } else if (curr.getIceBergLettuce() > 1) {
                    processOneIceburg(curr, sta);
                    processOneIceburg(curr, sta);
                }
                return;
            }
        }

    }

    private void processOneCarrot(Ingredients curr, Status sta) {
        sta.setSuccess(false);
        if (curr.getCarrots() > 0) {
            curr.setCarrots(curr.getCarrots() - 1);
            sta.setSuccess(true);
        } else {
            sta.setSuccess(false);
        }
    }

    private void processIceburg(Ingredients curr, Ingredients req, Status sta) {
        for (int i = 0; i < req.getIceBergLettuce(); i++) {
            processOneIceburg(curr, sta);
            if (!sta.isSuccess()) {
                return;
            }
        }
    }

    private void processOneIceburg(Ingredients curr, Status sta) {
        if (curr.getIceBergLettuce() > 0) {
            curr.setIceBergLettuce(curr.getIceBergLettuce() - 1);
            sta.setSuccess(true);
        } else {
            sta.setSuccess(false);
        }

    }

    private void processBeans(Ingredients curr, Ingredients req, Status sta) {
        for (int i = 0; i < req.getBeans(); i++) {
            processOneBean(curr, sta);
            if (!sta.isSuccess()) {
                if (curr.getIceBergLettuce() > 0) {
                    processOneIceburg(curr, sta);
                } else {
                    return;
                }
            }
        }

    }

    private void processOneBean(Ingredients curr, Status sta) {
        if (curr.getBeans() > 0) {
            curr.setBeans(curr.getBeans() - 1);
            sta.setSuccess(true);
        } else {
            sta.setSuccess(false);
        }
    }

    private void processOneChicken(Ingredients curr, Status sta) {
        if (curr.getChickens() > 0) {
            curr.setChickens(curr.getChickens() - 1);
            sta.setSuccess(true);
        } else {
            sta.setSuccess(false);
        }
    }

}

---

Data driven test code (junit or system integration or what to call this?):

package academic.cookSalad;

import static org.junit.Assert.assertEquals;

import org.junit.Test;
import org.s2n.ddt.util.DdtIoUtls;
import org.s2n.ddt.util.ExcelAccess;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

/**
 * Can use DdtUtils excel reader (which uses POI) to read an excel and run data cases
 * More pain to make but once made can add a variety of test cases
 * and validations per truth table at 
 * Not yet complete need to check impl code and test case data for accuracy.
 * TODO 
 * @author tushar
 *
 */
public class CookDataTest {
    private static final Logger logger = LoggerFactory.getLogger(CookDataTest.class);

    final static String SHT_NAME = "tests";
    final String TRUTH_TABLE_XLS = "./src/test/resources/TruthTableCookIt.xlsx";
    @Test
    public void data(){
        ExcelAccess xls = initExcelData();
        /*
         cook = new Cook();
        in = new Ingredients();
        in.setBeans(100);
        in.setIceBergLettuce(100);
        in.setChickens(100);
        in.setCarrots(100);
        Status st = cook.cookIt(in, Dish.CEASERS);
        assertTrue("Enough ingredients ceasrers ", st.isSuccess());
        assertEquals(1, 1);

         */

        Cook cook = new Cook();
        Ingredients in = new Ingredients();
        Dish dish = null;
        int r = 11;
        int c = 0;
        String typ = xls.getCellValue(SHT_NAME, r, c);
        int cases = 0;
        int pass = 0;
        while(typ != null && typ.length() > 0){
            cases++;
            typ = typ.toLowerCase();
            if(typ.equals("ceaser")){
                dish = Dish.CEASERS;
            }else{
                dish = Dish.RUSSIAN;
            }
            in.setCarrots(getInt(xls, r, 1));
            in.setChickens(getInt(xls, r, 2));
            in.setBeans(getInt(xls, r, 3));
            in.setIceBergLettuce(getInt(xls, r, 4));
            Status sta = cook.cookIt(in, dish);

            Status exp = parseExpected(xls, r);
            Ingredients out = exp.getLeftOverIngrediants();
            if(sta.isSuccess() == exp.isSuccess()){

                if(in.equalValues(out)){
                    pass++;
                }else{
                    CookTest.print("Expected", exp);
                    CookTest.print("Actual Status okay but left over ", sta);
                }
            }else{
                CookTest.print("Expected", exp);
                CookTest.print("Actual", sta);
            }
            r++;
            typ = xls.getCellValue(SHT_NAME, r, c);
        }
        System.err.println("Done " + cases + " pass " + pass);
        assertEquals("Total cases vs pass ", cases, pass);
    }

    private Status parseExpected(ExcelAccess xls, int r) {
        Status exp = new Status();
        Ingredients out = new Ingredients();
        out.setCarrots(getInt(xls, r, 6));
        out.setChickens(getInt(xls, r, 7));
        out.setBeans(getInt(xls, r, 8));
        out.setIceBergLettuce(getInt(xls, r, 9));
        exp.setLeftOverIngrediants(out);
        String sxp = xls.getCellValue(SHT_NAME, r, 10);
        boolean expb = "true".equals(sxp);
        exp.setSuccess(expb);
        return exp;
    }

    private int getInt(ExcelAccess x, int r, int c){
        String d = x.getCellValue(SHT_NAME, r, c);
        if(d != null && d.length() > 0){
            return Integer.parseInt(d);
        }
        return -1;
    }

    private ExcelAccess initExcelData(){
        DdtIoUtls.printCurrentFolderName();
        ExcelAccess xlsWk = new ExcelAccess();
        // /u/w/w4moh/CookSalad/src/test/resources/TruthTableCookIt.xlsx
        //final String full = "/u/w/w4moh/CookSalad/src/test/resources/TruthTableCookIt.xlsx";

        xlsWk.openWorkBook(new java.io.File(TRUTH_TABLE_XLS));
        String ss = xlsWk.getCellValue("Sheet1", 10, 3);
        logger.debug("Loaded Excel truth table ");
        System.out.println("Loaded excel value at 10,3 :" + ss + ". 2 :" + xlsWk.getCellValue("Sheet1", 9, 3) + ".");
        return xlsWk;

    }
}

Regular junit

package academic.cookSalad;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue;

import org.junit.Before;
import org.junit.BeforeClass;
import org.junit.Test;

/**
 * Unit test for cook salads.
 * A more modular approach would be to make a excel reader based impl
 */
public class CookTest

{
    Cook cook;
    Ingredients in;

    @BeforeClass
    public static void once(){
        System.out.println("@BeforeClass Any set up once per test class");

    }
    /**
     * Not neccessary but a nice way to initialize before every test
     * */
    @Before
    public void setup(){
        System.out.println("setup");
        cook = new Cook();
        in = new Ingredients();
        in.setBeans(100);
        in.setIceBergLettuce(100);
        in.setChickens(100);
        in.setCarrots(100);
    }

    @Test
    public void cookCeasersWithEnoughIngre() {

        Status st = cook.cookIt(in, Dish.CEASERS);
        assertTrue("Enough ingredients ceasrers ", st.isSuccess());
        assertEquals(1, 1);

    }

    /*
      While debugging can quickly disable other tests by commenting the annotation.
      So can focus on the problematic test.
      */
    @Test
    public void cookCeasersLess1Chicken() {
        in.setChickens(1);
        Status st = cook.cookIt(in, Dish.CEASERS);
        assertTrue("Enough ingredients ceasrers ", st.isSuccess());
        assertEquals(1, 1);

    }

    @Test
    public void cookRussianLess1Chicken() {
        in.setChickens(1);
        Status st = cook.cookIt(in, Dish.RUSSIAN);
        assertTrue("1 less chicken russian, enough carrots ", st.isSuccess());
        assertEquals(1, 1);
        print("less 1 chicken", st);

    }

    public static void print(String msg, Status st) {
        System.out.println(msg);
        System.out.println("[Success " + st.isSuccess());
        Ingredients out = st.getLeftOverIngrediants();
        System.out.println(", carrots " + out.getCarrots());
        System.out.println(", chicken " + out.getChickens());
        System.out.println(", beans " + out.getBeans());
        System.out.println(", ice burg " + out.getIceBergLettuce());
        System.out.println("]");

    }
}

Xls data src/test/resources/TruthTableCookIt.xlsx

Answer 1

Repeated Structure:

Cook has a number of cases where you a have a few options, but need to do the same operation with the options. This type of code does not scale and has already started to be a wart in the code.

processCarrrots(curr, req, sta);
if (sta.isSuccess()) {
    processBeans(curr, req, sta);
    if (sta.isSuccess()) {
        processChickens(curr, req, sta);
        if (sta.isSuccess()) {
            processIceburg(curr, req, sta);
            if (sta.isSuccess()) {
                sta.setLeftOverIngrediants(curr);
            }
        }
    }
}

What will you do if 1 more ingredient type needs to be supported? What about if 10 more are needed? Instead, you can make an interface that contains a method that knows how to process a specific ingredient. Then you could just iterate over a list of all the supported ingredients processing each one.

Ingredients req = new Ingredients();
if (what == Dish.CEASERS) {
    req.setCarrots(2);
    req.setIceBergLettuce(4);
    req.setChickens(1);
    req.setBeans(1);
} else {
    req.setCarrots(2);
    req.setBeans(2);
    req.setChickens(2);
    // req.setIceBergLettuceQty(0);//Not required
}

Are you just going to have a huge if/else block when more dishes are added? In addition, it is not the cook's responsibility to specify which ingredients are required for each dish. They are just in charge of preparing the food when the items are available. Instead, you could have the required set of ingredients be passed in as a parameter. The recipe is static. You are already passing in the Dish enum. You should add getRequiredIngredients() to Dish.

Ingredients req = what.getRequiredIngredients();

And you are done.

Note: what is a terrible variable name. It doesn't tell a reader anything. toMake is much more descriptive.

---

Naming Things:

The names used in Ingredients are inconsistent. Some times the method name is plural, some times it is singular. The variable names have "Qty", but the methods don't.

Status has leftOverIngrediants. However, you have to call setLeftOverIngrediants() before you try to cook something. They aren't left overs until you have finished cooking. Instead, availableIngredients would always be a valid name.

You have multiple spellings of ingredient.

---

Data Drive Test

There is way too much going on in this one method.

1. Importing a spreadsheet.
2. Converting imported data into usable inputs to your code.
3. Looping over each test case.
4. Actually preforming the check.
5. Writing out result information about the test case.

The testing framework should should be doing 3 and 5 for you. 1 and 2 belong in a @Before setup method. 4 is the only thing that should be the only thing happening with in a @Test method.

You should use Parameterized Test when you have multiple data points that need to be used for the same check (3). This way, if one case fails, all of the following cases are still executed. Knowing 6 of 10 cases are fail is much more helpful then knowing 1 case is failing, only to later find out there are more failures than that 1 case.

As for importing the data from an external source (1), I don't think this is a good practice. I prefer to keep the test data in the source code of the test. Using an external resource means:

• You have to go somewhere else to see what the data points are.
• Using a spreadsheet makes it harder to track changes to the data point in version control.
• The test might not actually run if there is a problem importing the resource.

Which brings us to converting the imported data (2). The farther your data points are from the test case, the more work it is to convert them.

The assertXXXX() methods are how JUnit tells you if a test passes or not (5). It will generate a report telling you which methods fail and pass. You can pass a message argument that will be included with the failure report when something is wrong. You also have a logger, yet you are using stdout.

---

CookTest

This is much better, but still has a number of issues.

Every test ends with:

    assertEquals(1, 1);

This will always pass and has nothing to do with your code. Remove it.

The test names are ok, but could be better. They describe the case being tested, but don't say what the expected result is. When you are just looking at a test report, being able to have a good idea of what is wrong by just seeing the test name is extremely helpful. It also makes the intent of the test clear to someone who is seeing the test for the first time. It is possible that there was a bug in the test itself.

A good name tells you: - What is being tested. - The context in which it is being tested. - The expected result.

Note: The name of your test in the data driven class is just data(). That doesn't satisfy any of the above point and could be confused with a method that just generates the data points to test.

---

Other things

• A copy constructor would save you the hassle of CloneNotSupportedException. clone() is only helpful when you don't know type of the thing you need a copy of. This can happen when there is subclass or an implementation of an interface.

• If you are using a logger, you shouldn't need to write things to stdout. Pick one way to generating output if you need more detailed information.

• Remove all of the dead code. The only thing it is doing is distracting people from what the actual code is doing. Additionally, it might not even compile any more if it were to be uncommented.

• There seems to be a number of comments that are telling the reader what a certain thing is used for. If the intent of this code is to be an example for how to use JUnit, that may be acceptable (but be consistent, don't use comments and writing to stdout). However, it should be noted that you generally shouldn't have comments saying what is happening. Comments are better at saying why something is happening. If the code is complicated enough that it needs to be described, it should be extracted into a function with a descriptive name.