Collect unique and the newest objects with Sets in Java

user_6788005 0
Viewed 74

Suppose I have a class (response model)

TransactionResult {
    private List<Transaction> transactions;
}

Transaction {
    private OffsetDateTime date;
    private String account;
}

and I want to have unique transactions by account and should be the newest by date. So, I have mapped dto

TransactionResultDto {
    private Set<TransactionDto> transactions;
}

TransactionDto implements Comparable<TransactionDto> {
    private OffsetDateTime date;
    private String account;
    
    //equals and hashcode only using account
    //compareTo using date
}

Firstly, I collect into a TreeSet to sort the transactions (newest are first) and then I build a HashSet to have unique transactions by account.

Set<TransactionDto> transactions = new HashSet<>(
                transactionResult.getTransactions().stream()
                        .map(transaction -> mapToTransactionDto(transaction))
                        .collect(Collectors.toCollection(TreeSet::new)));

So, the questions are:

  1. Is this the right way to collect the newest and unique by account transactions?
  2. Do you have any improvements or other ideas?
set java
2 Answers

1. No, it is incorrect to use TreeSet for this case.

Suppose we have transactions:
{"account": "A", "date": "2022-01-01"},
{"account": "B", "date": "2022-01-01"}(omit time for simplicity).

They are considered the same object if you put them in TreeSet as you only compare date.(i.e. only 1 objects remain if you put both in TreeSet). What you may face is some account records may disappear when the date collides with another account. It may be extremely rare but it will cost you days to debug if it really occurs.

2. Let's fix it and make it more descriptive

Your method can also be fixed by using sort, and then put it to HashSet. However, this approach has some drawbacks:

  • The logic is not straightforward, as the reader needs time to figure out what it is trying to achieve.
  • Override hashcode and equals to consider account only may cause trouble when other users expect both account and date should be considered.

As what you really want is

  1. groupBy account
  2. select transaction with max date in each group.

We can use Collectors#groupingBy and then select Collectors#maxBy for downstream, as shown below:

Set<TransactionDto> transactions = transactionResult.getTransactions().stream().map(TransactionDto::new)
       .collect(
               groupingBy(TransactionDto::getAccount,
                       collectingAndThen(
                               maxBy(Comparator.comparing(TransactionDto::getDate)),
                               Optional::get)
               )
       ).values().stream()
       .collect(toSet());
user_79287210

I would probably just loop over them in one pass. You can use a HashMap to store the newest for each account and then build a set from the HashMap's values.

You asked for other tips so I would:

  1. Have a method that just finds the transactions you want. Converting to DTO or anything else can be handled elsewhere.
  2. Have equals and hash use all the fields, otherwise it won't be useful for other use cases. For example, I wrote some unit tests for my method below and having equals and hash using all fields was very useful.
  3. I would personally not implement comparator here for this task. You can imagine other useful ways of ordering these for different tasks and I don't think this object really has an intrinsic order. You can always implement as many custom comparators as you like but I personally wouldn't wouldn't do this either for this task.
  4. Without knowing much about the architecture of your system, I would expect DTOs to be for transferring data and domain objects to have functionality, so I would move logic to those classes. Your architecture might be different.

To build a collection of the newest transactions, I would probably do something like this:

private Collection<Transaction> findNewestForEachAccount(Collection<Transaction> transactions) {
    HashMap<String, Transaction> result = new HashMap<>();
    for (Transaction t : transactions) {
        if (isNewestForAccountSoFar(result, t)) {
            result.put(t.getAccount(), t);
        }
    }
    return result.values();
}

private boolean isNewestForAccountSoFar(HashMap<String, Transaction> result, Transaction t) {
    return !result.containsKey(t.getAccount())
        || isNewer(t, result.get(t.getAccount()));
}

private boolean isNewer(Transaction candidate, Transaction incumbent) {
    return candidate.getDate().isAfter(incumbent.getDate());
}

You could then build your DTOs like this:

    List<Transaction> transactions = transactionResult.getTransactions();
    Set<TransactionDto> selectedTransactionDTOs = findNewestForEachAccount(transactions)
        .stream()
        .map(t -> mapToTransactionDto(t))
        .collect(Collectors.toSet());

One option would be to move the logic for finding the newest transactions to your TransactionResult class. This would mean you wouldn't have to expose the getter for the entire list. This depends on your architecture though so your mileage may vary.

user_65161780
Related