0

I have following method which is used for creating a order in the database, order has many items and, item has many bills. iPadPOSOrderDTO is the order which is going to base saved into the database.

so, the loop based code for creating order is the following

 private void createNewOrder(IPadPOSOrderDTO iPadPOSOrderDTO) {
    IPadPOSOrderV2 order = mapper.map(iPadPOSOrderDTO, IPadPOSOrderV2.class);
    if(order.getOrderV2Bills()!=null && order.getOrderV2Bills().size()>0){
        for(IPadPOSOrderV2Bill orderBill : order.getOrderV2Bills()){
            orderBill.setOrder(order);

            if(orderBill.getiPadPOSOrderV2BillItems()!=null && orderBill.getiPadPOSOrderV2BillItems().size()>0){
                for(IPadPOSOrderV2BillItems orderBillItem :  orderBill.getiPadPOSOrderV2BillItems()){
                    orderBillItem.setiPadPOSOrderV2Bill(orderBill);
                    orderBillItem.setOrderId(order.getOrderId());

                }
            }
        }
    }

    sessionFactory.
            getCurrentSession().save(order);
}

I wanted to refactor above code to use Java 8 streams API.

So, I did the following 

private void createNewOrderV2(IPadPOSOrderDTO iPadPOSOrderDTO) {
        IPadPOSOrderV2 order = mapper.map(iPadPOSOrderDTO, IPadPOSOrderV2.class);
        if(order.getOrderV2Bills()!=null && order.getOrderV2Bills().size()>0){
            order.getOrderV2Bills().stream().forEach(e -> { createBill(order,e);});
        }
        sessionFactory.
                getCurrentSession().save(order);
    }

    private void createBill(IPadPOSOrderV2 ipadExistingOrderFromDatabase, IPadPOSOrderV2Bill iPadPOSOrderV2Bill) {
        iPadPOSOrderV2Bill.setOrder(ipadExistingOrderFromDatabase);

        if(iPadPOSOrderV2Bill.getiPadPOSOrderV2BillItems()!=null && iPadPOSOrderV2Bill.getiPadPOSOrderV2BillItems().size()>0){
            iPadPOSOrderV2Bill.getiPadPOSOrderV2BillItems().stream().forEach(e -> createBillItem(ipadExistingOrderFromDatabase,iPadPOSOrderV2Bill,e));
        }
    }

    private void createBillItem(IPadPOSOrderV2 ipadExistingOrderFromDatabase, IPadPOSOrderV2Bill iPadPOSOrderV2Bill, IPadPOSOrderV2BillItems iPadPOSOrderV2BillItem) {
        iPadPOSOrderV2BillItem.setiPadPOSOrderV2Bill(iPadPOSOrderV2Bill);
        iPadPOSOrderV2BillItem.setOrderId(ipadExistingOrderFromDatabase.getOrderId());
        ipadExistingOrderFromDatabase.getOrderV2Bills().stream().forEach(e -> { createBill(ipadExistingOrderFromDatabase,e);});
    }

could somebody share their experience and advice me if I am making the correct use of streams API for this refactoring.

Improve this question
4
  • 1
    Why do you want to convert that code to streams? It should be easier to do than your current version but have a look at the code you produced and understand: did it actually make it more readable for you? Commented Mar 13, 2020 at 9:10
  • non-stream version is more readable IMO, you don't need to check order.getOrderV2Bills().size() and orderBill.getiPadPOSOrderV2BillItems().size() but the way use of isEmpty() method is better than size() Commented Mar 13, 2020 at 9:18
  • @Thomas because I wrote both code segments, it does not make any difference. I am able to read both code segments easily. But, stream based code looks sequential to me and able to comprehend easily if I am reading code written by other developer. Other point is, I want to learn how to use streams in this context. Commented Mar 13, 2020 at 9:19
  • Well, yes since you've written both versions you should be able to understand them. What I was getting at was would using streams make it really that much easier to understand? If it is for learning purposes then that's a good enough reason for me but I'd caution against using streams just to have used streams. If there are no functional reasons (like wanting to use parallel streams) use whatever you and others will be able to understand more easily in 6 months from now (that might even be more code if it's easier to read). Commented Mar 13, 2020 at 9:48

2 Answers 2

Reset to default
1

Note that those size checks aren't really necessary. An empty list would result in an empty stream and thus nothing would get applied. The only benefit would be that you'd be able to avoid having to create the stream altogether but I highly doubt the performance difference would even be noticeble.

If you want to convert a potentially null collection to a stream you might want to use a small helper function:

public <T> Stream<T> collectionToStream(Collection<T> collection) {
  return Optional.ofNullable(collection).map(Collection::stream).orElseGet(Stream::empty);
}

Using forEach() you could then do something like this:

private void createNewOrder(IPadPOSOrderDTO iPadPOSOrderDTO) {
  IPadPOSOrderV2 order = mapper.map(iPadPOSOrderDTO, IPadPOSOrderV2.class);
  collectionToStream(order.getOrderV2Bills()).forEach( orderBill -> {
      orderBill.setOrder(order);

      collectionToStream(orderBill.getiPadPOSOrderV2BillItems()).forEach(orderBillItem -> {
          orderBillItem.setiPadPOSOrderV2Bill(orderBill);
          orderBillItem.setOrderId(order.getOrderId());
        }
      }
    }
  }   

  sessionFactory.getCurrentSession().save(order);
}

Note that this isn't that different from your initial code and thus you should think about whether that conversion would make sense.

Converting your nested loops to a fully sequential stream would be harder and in the end not that different because you can't just flat map orderBill to a stream of orderBillItem. Doing that would not make orderBill available downstream so you'd have to call orderBillItem.setiPadPOSOrderV2Bill(orderBill); before returning the nested stream. That would end up in code very similar to the above and add no benefit because you're not using the returned stream.

Improve this answer
Sign up to request clarification or add additional context in comments.

4 Comments

IMHO, that helper you wrote could just be an abuse of the Optional and Collection at the same time. One should refrain from ending up with such a helper by implementations.
@Naman you're right in that such helpers should be avoided. Unfortunately you sometimes want to avoid those null checks on collections that could be null (and which you can't change anything about) and there's no method in Stream itself that would return an empty stream for null collections (something like Stream.ofNullable() would be great if it would allow for collections, e.g. Stream.ofNullable(Collection<T>) or maybe better Collections.streamOfNullable(Collection<T>)).
There is a Stream.ofNullable that dos exist since Java-9, but that would further require a flatMap to get the actual stream of data. Anyway, the cause wouldn't still be solved.
@Naman you're right, Stream.ofNullable() in conjunction with flatMap() would probably make it clearer. And yes, the cause for needing to do that wouldn't be solved - the problem is that I we can't assume the OP will be able to solve that at all.
0

Filter out the nulls ommiting the null checks

     private void createNewOrderV2(IPadPOSOrderDTO iPadPOSOrderDTO) {
          IPadPOSOrderV2 order = mapper.map(iPadPOSOrderDTO, IPadPOSOrderV2.class);
          order.getOrderV2Bills().stream().filter(Objects::nonNull).forEach(e -> createBill(order, e));
          sessionFactory.getCurrentSession().save(order);
       }

       private void createBill(IPadPOSOrderV2 ipadExistingOrderFromDatabase, IPadPOSOrderV2Bill iPadPOSOrderV2Bill) {
          iPadPOSOrderV2Bill.setOrder(ipadExistingOrderFromDatabase);
          iPadPOSOrderV2Bill.getiPadPOSOrderV2BillItems().stream().filter(Objects::nonNull).forEach(e -> {
                e.setiPadPOSOrderV2Bill(iPadPOSOrderV2Bill);
                e.setOrderId(ipadExistingOrderFromDatabase.getOrderId());
          });
       }

By the way your createBill() is called by the createBillItem and also the other way around, is this correct?

Improve this answer

1 Comment

How is order.getOrderV2Bills().stream().filter(Objects::nonNull) meant to help with order.getOrderV2Bills() potentially returning null? What do you think order.getOrderV2Bills().stream() would do in that case?

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.