1
\$\begingroup\$

I use two lists of objects invoices, original list and a new modified list. I would insert in db invoices presents in the new modified list but not presents in the original list.

For delete, I would remove from db invoices presents in the original list but absent in the new modified list.

This is what I did and that works, but I am not satisfied with the algorithm used, someone would have a way to do this more simply and clearly?

public void updateCustomerInvoicesCouple(List<CustomerInvoicesDto> originalCouplesListDto, List<CustomerInvoicesDto> newCoupleListDto){

    // 1 - Insert
    Iterator<CustomerInvoicesDto> itFinal = newCoupleListDto.iterator();

    // Browse all couples (customer-invoice) from final list
    // Insert couples of final list not in original list
    while(itFinal.hasNext()){
        CustomerInvoicesDto finalCoupleDto = (CustomerInvoicesDto) itFinal.next();                      

        Integer idCustomerFinal = finalCoupleDto.getCustomer().getId();
        Integer idInvoiceFinal = finalCoupleDto.getInvoice().getId();           

        boolean coupleFind = false;
        for(CustomerInvoicesDto itemInitialCouple : originalCouplesListDto)             
        {                       
            if(idCustomerFinal == itemInitialCouple.getCustomer().getId() 
                    && idInvoiceFinal == itemInitialCouple.getInvoice().getId())
            {
                coupleFind = true;
            }                   
        }

        if(!coupleFind){
            // dto -> entity
            CustomerInvoicesEntity finalCoupleEntity = new CustomerInvoicesEntity();
            finalCoupleEntity = mapper.map(finalCoupleDto, CustomerInvoicesEntity.class);

            // Save in db
            CustomerInvoicesDao.save(finalCoupleEntity);
        }
    }

    // 2 - Delete
    Iterator<CustomerInvoicesDto> itInit = originalCouplesListDto.iterator();

    // Browse all couples (customer-invoice) from original list
    // Insert couples of final list not in final list
    while(itInit.hasNext()){            
        CustomerInvoicesDto initialCoupleDto = (CustomerInvoicesDto) itInit.next();                     

        Integer idCustomerInitial = initialCoupleDto.getCustomer().getId();
        Integer idInvoiceInitial = initialCoupleDto.getInvoice().getId();           

        boolean coupleFind = false;
        for(CustomerInvoicesDto itemFinalCouple : newCoupleListDto)             
        {                       
            if(idCustomerInitial == itemFinalCouple.getCustomer().getId() 
                    && idInvoiceInitial == itemFinalCouple.getInvoice().getId())
            {
                coupleFind = true;
            }                   
        }

        if(!coupleFind){
            CustomerInvoicesDao.remove(initialCoupleDto.getId());
        }
    }       
}
\$\endgroup\$
1
\$\begingroup\$

1) In your for-loop, when the if is true, you should not only set coupleFind = true, but also add a break since you keep looping for nothing otherwise.

2) I'm not sure why the customerId is not already part of the invoice.

3) Instead of looping over the lists, you could just create a HashSet and check if the invoice is already present. There is some extra complexity if you take this approach, but if you call that method often, it will definitely be cheaper computationally. You should define some class InvoiceIncludingCustomerId if you don't want to put the customerId within the invoice. And you should override (carefully) the hashCode() and equals() of that class so that the hash is unique, but quick to compute: invoiceId and customerId should be sufficient.

You then just need a HashSet<InvoiceIncludingCustomerId> knownInvoices and you just check if an invoice exists with knownInvoices.contains(invoice). If it is missing, you add it to the DB and knownInvoices.add(invoice).

\$\endgroup\$
0
\$\begingroup\$

Here's how I would personally do it :

public void updateCustomerInvoicesCouple(List<CustomerInvoicesDto> originalCouplesListDto, List<CustomerInvoicesDto> newCoupleListDto){
        Map<Pair<Integer, Integer>, CustomerInvoicesDto> originalCouplesByIds = new HashMap<>();
        List<CustomerInvoicesDto> addedCouples = new ArrayList<CustomerInvoicesDto>();

        for (CustomerInvoicesDto originalCouple : originalCouplesListDto) {
            originalCouplesByIds.put(new Pair<Integer, Integer>(originalCouple.getCustomer().getId(),  originalCouple.getInvoice().getId()), originalCouple);
        }

            // at the end of this loop, the originalList will contain list that have been deleted.
        for (CustomerInvoicesDto newCouple : newCoupleListDto) {
            if (null == originalCouplesByIds.remove(new Pair<>(newCouple.getCustomer().getId(),  newCouple.getInvoice().getId()))) {
                            // if the original list contains the Ids, then it's not an insert nor a delete. If it doesn't, it's an insert
                addedCouples.add(newCouple);
            }
        }

        insertAll(addedCouples);
        deleteAll(originalCouplesByIds.values());
    }

I would also suggest to not use integers as Ids. Create your own Id class and use this instead. Or worst case, use a String.

\$\endgroup\$
  • \$\begingroup\$ Why do you think that String is better for ids than Integer? \$\endgroup\$ – svick Sep 10 '13 at 10:48
  • \$\begingroup\$ It's more flexible. If you want to make your IDs unique across types (tables), integer won't do the trick. A String can always change in DB without needing to change most of the code \$\endgroup\$ – molyss Sep 10 '13 at 13:18

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.