CODEWITHSUNDEEP
javalistfilter
Søren Pedersen
Søren Pedersen

Reputation: 21

Remove items from inner list

I have a list of departments with a list of addresses. Each address has a period with a from date. The following code finds the address within each department with the latests from date and removes all other addresses. It seems to work, but I'm sure it can be done in a more elegant way. Any suggestions?

List<Department> departments = fetchDepartments();

departments.forEach(department -> {
  
  Date fromDate = department.getAddresses().stream().map(address -> 
  address.getPeriod().getFromDate()).max(Date::compareTo).get();
            
  department.setAddresses(department.getAdresses().stream()
    .filter(address -> address.getPeriod().getDateFrom().equals(fromDate))
    .collect(Collectors.toList()));
});

Upvotes: 0

Views: 33

Answers (2)

azro
azro

Reputation: 54148

You could use a TreeMap to collect the adresses regarding their fromDate, because as the TreeMap sorts automatically, you can retrieve the last key, meaning the highest one

for (Department department : departments) {
    TreeMap<Date, List<Adress>> fromDate = new TreeMap<>(department.getAdresses().stream()
        .collect(Collectors.groupingBy(adress -> adress.getPeriod().getFromDate())));

    department.setAdresses(fromDate.lastEntry().getValue());
}

Upvotes: 1

rzwitserloot
rzwitserloot

Reputation: 102902

Your code isn't functional in the first place (you have a setter), so why use the rather gimped .forEach terminal here? It's not shorter, you gain nothing, and you lose exception transparency, local variable transparency, and control flow transparency.

When there are 2 ways to do a thing, and one of the two ways is strictly less capable, don't use that. Also, your code will fail if any department has zero addresses inside; surely the right move in that scenario is to simply not do anything. Also, if there are 2+ addresses with the same 'from date', you save all of them. Is that intentional or would you prefer that an arbitrary one is chosen instead.

for (Department d : fetchDepartments()) {
  var addresses = department.getAddresses();
  if (addresses.size() < 2) continue;
  var address = addresses.stream()
    .max(Comparator.comparing(a -> a.getPeriod().getFromDate()))
    .get();
  department.setAddresses(List.of(address));
}

This does a lot more (such as avoid a bunch of footwork if there wasn't a point in the first place and avoid exceptions).

It's also significantly shorter in spite of the fact that it does more.

Upvotes: 1

Related Questions