TDD and use case: cook dish with substitutions

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 CloneNotSupp

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.

• Importing a spreadsheet.

• Converting imported data into usable inputs to your code.

• Looping over each test case.

• Actually preforming the check.

• 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 `CloneNotSuppor

StackExchange Code Review Q#75853, answer score: 5