Designing object oriented clean code structure with spring services

Question

I'm writing spring app for storing and manipulating recipes. I wrote ShoppingList service which generates, based on UUIDs of recipes, shopping list. This is my current model for which i wrote tests and works as intended.

Service uses another service called IngredientConverter which converts to response models. Now i wanted to move some dependency to ShoppingList as this function is jealous of functions, but approached a problem. How can a this class use IngredientConverter service without ugly passing it to ShoppingList function? Are there any tips you could give my code and structure planning?

@Service
@AllArgsConstructor
public class ShoppingListService {

    private final RecipeService recipeService;
    private final IngredientConverter ingredientConverter;

    public ShoppingList generateShoppingList(List<UUID> uuidsOfRecipes) {
        List<Recipe> recipes = recipeService.getAllByIDIn(uuidsOfRecipes);
        ShoppingList shoppingList = ShoppingList.empty();

        Map<Ingredient, Integer> ingredients = new HashMap<>();

        recipes.forEach(recipe ->
                recipe.getIngredients().forEach(
                        ingredientQuantity ->
                                ingredients.compute(
                                        ingredientQuantity.getIngredient(),
                                        (key, value) ->
                                                value == null ?
                                                        ingredientQuantity.getAmount() :
                                                        value + ingredientQuantity.getAmount())));

        ingredients.keySet().forEach(
                ingredient ->
                        shoppingList.getIngredients().add(
                                ingredientConverter.convertWithAmount(
                                        ingredient.getName(),
                                        ingredients.get(ingredient),
                                        ingredient.getUnit())
                        )

        );

        return shoppingList;
    }

}

@Data
@AllArgsConstructor
public class ShoppingList {

    private final List<IngredientQuantity> ingredients;

    public static ShoppingList of(List<IngredientQuantity> ingredients) {
        return new ShoppingList(ingredients);
    }

    public static ShoppingList empty(){
        return ShoppingList.of(new ArrayList<>());
    }

}

@Service
public class IngredientConverter {

 public IngredientQuantity convertWithAmount(String name, int amount, Unit unit) {
        return IngredientQuantity.builder()
                .amount(amount)
                .ingredient(convert(name, unit))
                .build();
    }
}

@Data
@Entity
@SuperBuilder
@NoArgsConstructor
@AllArgsConstructor
@EqualsAndHashCode(callSuper = true)
public class IngredientQuantity extends IdentifiableEntity {

    private int amount;
    @ManyToOne
    private Ingredient ingredient;
}

@Data
@Getter
@Entity
@SuperBuilder
@NoArgsConstructor
@AllArgsConstructor
@EqualsAndHashCode(callSuper = false)
public class Ingredient extends IdentifiableEntity {

    private String name;
    @Enumerated(EnumType.STRING)
    private Unit unit;
}

@Getter
@NoArgsConstructor
@SuperBuilder
@MappedSuperclass
public class IdentifiableEntity {

    @Id
    @GeneratedValue(strategy = GenerationType.AUTO)
    private UUID ID;
}
```

Answer 1

I would inject the converterservice into ShoppingList and implement an add() function, which adds an element to the ShoppingList after conversion.

void add(name, ingredient, unit) {
    IngredientQuantity toAdd = this.converterService.convert(name, ingredient, unit); // terrible naming, todo!
    this.ingredients.add(toAdd);
}

Now the shopping list is the owner of conversion which (arguably) is modelling real life. When I need to shop for milk, I convert the American recipes to litres instead of ounces or whatever.

This also simplifies your last lambda function, you just need to iterate over the keyset and call ShoppingList::add, which is Java8+ method ref syntax.

The other huge lambda should probably also be split into parts, I mean, I love functional java as much as anyone, but I believe this is cutting the line of functional utility over readability. Pseudocode

foreach Recipe:
    foreach Ingredient : Recipe
        compute(ingredient)

These are my 2 cents.

Answer 2

Streams

A couple things to keep in mind.

• The new syntax for .forEach() is cool but it should be used for simple things and it should not be nested. Either refactor it to avoid nesting or extract the nested parts to methods. This way those parts will have names which should make their purpose obvious.

• Don't modify collections with .forEach. If you need to process a collection use streams and collectors to construct the collection you desire. This will eliminate the temporal coupling between creating empty collections and filling them up.

    public ShoppingList generateShoppingList(List<UUID> uuidsOfRecipes) {
        List<Recipe> recipes = recipeService.getAllByIDIn(uuidsOfRecipes);
        Map<Ingredient, Integer> ingredients = recipes.stream()
            .flatMap(recipe -> recipe.getIngredients().stream())
            .collect(Collectors.toMap(
                IngredientQuantity::getIngredient,
                IngredientQuantity::getAmount,
                Integer::sum));
  
        # Next step would be to refactor this piece.
        ShoppingList shoppingList = ShoppingList.empty();
        ingredients.forEach((ingredient, amount) -> shoppingList.getIngredients().add(
            ingredientConverter.convertWithAmount(
                ingredient.getName(),
                amount,
                ingredient.getUnit())
        ));
  
        return shoppingList;
    }
  

Note: The code is untested.

General

• I would suggest dropping the IngredientConverter. It serves no purpose since in can be replaced by a constructor.
• The only object with any meaningful logic is ShoppingListService. You could make your code more object oriented by delegating some logic to other parts. For example Shopping list can be responsible for aggregation of ingredience. Just implement add(IngredientQuantity) on it and let it handle the ingredient totals. This would further simplify the service since it won't have to convert to maps and calculating the total there.