Refactoring Kung-Fu, Part VI

Ok, time to run the test.

Well, that’s interesting. The test is not even running until the end, we’re crashing before it completes. Let’s see what’s wrong.

Seems that this line breaks:

List<Ingredient> fillingIngredients = new ArrayList<Ingredient>(ingredients.subList(1, ingredients.size() - 1));

when running the case where dish includes: Sauce = Pesto, Pasta = Ravioly. In this case, the sublist method is invoked with (1,0) which causes the IllegalArgumentException. Turns out sublist‘s fromIndex (the first parameter) should be smaller than the toIndex (the second). I bet that never happens in legacy code.

What happens now? We’ve clearly identified a case where the code breaks. It is one of these “how did it ever work” moments. What do we do?

Let’s investigate a bit more. Let’s run each case separately to see which cases cause the crash, and which just fail the approval. Is this a big problem, or a single case problem?

Turns out it just this case, all other cases don’t crash. What do we do? Well, we’ve identified a case that breaks the system, and we still don’t know enough about the system to create a test for it.

We can rely on the “how did it ever work” feeling, and therefore think: This case doesn’t occur in the wild, (otherwise we would have known about it), there’s not much sense in keeping it in check. As part of an investigation in the real world legacy code, I would try to reproduce the breaking case in the system, or at least understand why it hasn’t occurred yet.

What do we do?

We can disregard that case, and not use it as a characterization of the system. All other cases don’t cause breakage, and we can work with them.

And yet, it doesn’t feel right. The behavior exists in the code, and it has some kind of effect (the breaking kind). Maybe we want to make sure that although this is a breaking behavior, it is still a behavior we want to maintain. In that case, we can modify the test to something like this:

@Test
public void pastaMakerTest() {
    MockDispenser dispenser = new MockDispenser();
    StringBuilder log = new StringBuilder();
    PastaMaker maker = new PastaMaker(dispenser);

    dishes.forEach(dish -> {
      log.append(dish.toString());
      try {
        maker.cook(dish.sauce, dish.pasta);
      }
      catch (Exception e) {
        log.append(e.getMessage());
      }
    });

    log.append(dispenser.toString());
    Approvals.verify(log);
}

This way, we also log the error as part of the process, and in our refactoring preserve the behavior.

We can also modify the PastaMaker code to not break (swallow the exception, fix it ad-hoc, put the offending line inside a try-catch block and handle it there, or any other creative method). While this is a valid option, it is risky, because what we’re actually doing is not refactoring, we’re changing functionality without safeguards.

The option I chose is to disregard the breaking case for now, and add a test for it later. I’m planning to come back to it and make sure it is covered, once I know more about it.

Also, if we leave the approval tests as part of the CI, I’m not sure I want to preserve an exception throwing behavior. While it is the current behavior, I don’t intend to leave it like that eventually, which goes back to writing that test later.

So for now, I’m commenting out the last line of the dish list:

List<Dish> dishes = List.of (
    new Dish(SauceType.Alfredo, PastaType.FreshSpaghetti),
    new Dish(SauceType.Bolognese, PastaType.FreshSpaghetti),
    new Dish(SauceType.Marinara, PastaType.FreshSpaghetti),
    new Dish(SauceType.Pesto, PastaType.FreshSpaghetti),
    new Dish(SauceType.Alfredo, PastaType.Ravioly),
    new Dish(SauceType.Bolognese, PastaType.Ravioly),
    new Dish(SauceType.Marinara, PastaType.Ravioly)
//    new Dish(SauceType.Pesto, PastaType.Ravioly)
);

Next: Explore the output.