multiple operation on list data using stream api

Question

I have a requirement to calculate the basket price. I am getting list of items as an input, and first calculating the price of each item using map. After that I have to calculate the basket total and return ShoppingBasketOutput. I have attached my code below, but I don't like the idea of passing ShoppingBasketOutput into calculateBasketPrice as a param.

Do you a better suggestion to implement this requirement?

public ShoppingBasketOutput getBasketTotalPrice(List<ShoppingItemDTO> itemsDTO) {
    ShoppingBasketOutput shoppingBasketOutput = new ShoppingBasketOutput();

    itemsDTO.stream()
            .map(this::calculateItemPrice)
            .forEach((item) -> calculateBasketPrice(shoppingBasketOutput, item));
    return shoppingBasketOutput;
}

private ShoppingBasketOutput calculateBasketPrice(ShoppingBasketOutput shoppingBasketOutput, ShoppingItemDTO item) {
    shoppingBasketOutput.setSubTotal(shoppingBasketOutput.getSubTotal() + item.getItemSubTotal());
    shoppingBasketOutput.setTotalDiscount(shoppingBasketOutput.getTotalDiscount() + item.getTotalItemDiscount());
    return shoppingBasketOutput;
}

private ShoppingItemDTO calculateItemPrice(ShoppingItemDTO shoppingItemDTO) {
    Optional<ItemEnum> itemEnum = ItemEnum.getItem(shoppingItemDTO.getName());
    if (itemEnum.isPresent()) {
        ItemEnum item = itemEnum.get();
        shoppingItemDTO.setTotalItemDiscount(getItemTotalDiscount(item, shoppingItemDTO.getQuantity()));
        shoppingItemDTO.setItemSubTotal(item.getPrice() * shoppingItemDTO.getQuantity());
    } else {
        throw new ProductNotFoundException();
    }
    return shoppingItemDTO;
}

Answer 1

You're going at it the wrong way.

First of all remember that map(mapper) requires the mapping function to be:

a non-interfering, stateless function to apply to each element

In your case, it is not non-interfering and stateless because you are modifying the Stream element, which is your ShoppingItemDTO. So this is bad.

How can we correct this? Well, we need to think about what we want to achieve here. Your code calculates two values for each ShoppingItemDTO and then sums them all together to produce a ShoppingBasketOutput.

So, what you want here is to reduce your itemsDTO into a ShoppingBasketOutput. The identity to use is a new empty ShoppingBasketOutput. Everytime we encounter an item, we add to the basket output its subtotal and its discount. The third parameter combines two basket output together by summing their values.

public ShoppingBasketOutput getBasketTotalPrice(List<ShoppingItemDTO> itemsDTO) {
    return itemsDTO.stream().reduce(
        new ShoppingBasketOutput(),
        (output, item) -> {
            ItemEnum itemEnum = ItemEnum.getItem(item.getName()).orElseThrow(ProductNotFoundException::new);
            output.setSubTotal(output.getSubTotal() + itemEnum.getPrice() * item.getQuantity());
            output.setTotalDiscount(output.getTotalDiscount() + getItemTotalDiscount(itemEnum, item.getQuantity()));
            return output;
        },
        (output1, output2) -> {
            output1.setSubTotal(output1.getSubTotal() + output2.getSubTotal());
            output1.setTotalDiscount(output1.getTotalDiscount() + output2.getTotalDiscount());
            return output1;
        }
    );
}

Notice that you could this code a lot cleaner by introducing two methods addToSubTotal(amount) and addToTotalDiscount(amount). With such methods, you would need to get and set the new values.

Answer 2

This is same answer as that of Tunaki, which I have marked as accepted answer, and refactored it little bit by moving the logic into separate methods rather than everything into same method.

public ShoppingBasketOutput getBasketTotalPrice(List<ShoppingItemDTO> itemsDTO) {
    return itemsDTO.stream()
            .reduce(
                    new ShoppingBasketOutput(),
                    this::calculateItemPrice,
                    this::calculateBasketPrice
            );
}

private ShoppingBasketOutput calculateBasketPrice(ShoppingBasketOutput firstOutput, ShoppingBasketOutput secondOutput) {
    firstOutput.setSubTotal(firstOutput.getSubTotal() + secondOutput.getSubTotal());
    firstOutput.setTotalDiscount(firstOutput.getTotalDiscount() + secondOutput.getTotalDiscount());
    return firstOutput;
}

private ShoppingBasketOutput calculateItemPrice(ShoppingBasketOutput output, ShoppingItemDTO item) {
    ItemEnum itemEnum = ItemEnum.getItem(item.getName()).orElseThrow(ProductNotFoundException::new);
    output.setSubTotal(output.getSubTotal() + itemEnum.getPrice() * item.getQuantity());
    output.setTotalDiscount(output.getTotalDiscount() + getItemTotalDiscount(itemEnum, item.getQuantity()));
    return output;
}

Answer 3

Assuming you've got the constructors available and don't want to create any other classes:

public ShoppingBasketOutput getBasketTotalPrice(List<ShoppingItemDTO> itemsDTO) {
    ShoppingItemDTO res = itemsDTO.stream().map(this::calculateItemPrice).reduce(new ShoppingItemDTO(0, 0),
            (l, r) -> new ShoppingItemDTO(l.getItemSubTotal() + r.getItemSubTotal(),
                    l.getItemTotalDiscount() + r.getItemTotalDiscount()));

    return new ShoppingBasketOutput(res.getItemSubTotal(), res.getItemTotalDiscount());
}

Answer 4

Why is calculateItemPrice returning it's argument. It could be void.

Try this (not tested)

public ShoppingBasketOutput getBasketTotalPrice(List<ShoppingItemDTO> itemsDTO) {
    ShoppingBasketOutput shoppingBasketOutput = new ShoppingBasketOutput();

    itemsDTO.forEach(this::calculateItemPrice);

    shoppingBasketOutput.setSubTotal(
      itemsDTO.stream().mapToDouble(ip -> ip.getSubTotal()).sum()
    );

    shoppingBasketOutput.setTotalDiscount(
      itemsDTO.stream().mapToDouble(ip -> ip.getTotalDiscount()).sum()
    );

    return shoppingBasketOutput;
}