2
\$\begingroup\$

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;
}
```
\$\endgroup\$
  • 1
    \$\begingroup\$ The current question title of your question is too generic to be helpful. Please edit to the site standard, which is for the title to simply state the task accomplished by the code. Please see How do I ask a good question?. \$\endgroup\$ – BCdotWEB Sep 25 at 13:49
  • \$\begingroup\$ The only thing Lombok's autogenerated methods provide to me is frustration when I try to find which classes use a certain field of a data class. It doesn't save typing because it takes longer to write the annotation than it takes to autogenerate the ccessor methods in an IDE. Please reconsider your decision of using it. It may seem like a nifty tool at first but in the long run it is simply counterproductive. \$\endgroup\$ – TorbenPutkonen Sep 28 at 6:07
1
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thank you for response, should see service be injected in constructor, and should I just pass it to my static constructor from service? \$\endgroup\$ – wokstym Sep 25 at 14:02
  • \$\begingroup\$ Inject the service into a ShoppingList constructor, so that it autowires. \$\endgroup\$ – Edwin Carlsson Sep 25 at 14:04
  • \$\begingroup\$ And could you elaborate on terrible naming in your comment? How could naming here be improved? \$\endgroup\$ – wokstym Sep 25 at 14:15
  • \$\begingroup\$ toAdd is not an illuminating name. Maybe ingredient? \$\endgroup\$ – Edwin Carlsson Sep 25 at 14:20
  • \$\begingroup\$ Aa okay, I thought you commented on my service function, thx for replies \$\endgroup\$ – wokstym Sep 25 at 15:04
0
\$\begingroup\$

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.
| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.