14
\$\begingroup\$

I am writing a Java function that checks if the items of a list are unique (depending of an id member).

I have a Java 7 implementation that works, but it is not using streams, filter and joins. I think the code can be improved.

Case 1: empty list => OK

Case 2: all ids are different => OK

+---------+-------+
| baf18c4 | Item1 |
| c5e4623 | Item2 |
| bf1932f | Item3 |
+---------+-------+

No message expected.

Case 3: one id is duplicate (3 times each) => message expected

+---------+-------+
| baf18c4 | Item1 |
| c5e4623 | Item2 |
| baf18c4 | Item3 |
| baf18c4 | Item4 |
+---------+-------+

Expected message:

Some of the items are sharing the same code: 'baf18c4' used by {Item1, Item3, Item4}

Case 4: two ids is duplicate (2 times each) => message expected

+---------+-------+
| baf18c4 | Item1 |
| c5e4623 | Item2 |
| baf18c4 | Item3 |
| bf1932f | Item4 |
| c5e4623 | Item5 |
+---------+-------+

Some of the items are sharing the same code: 'baf18c4' used by {Item1, Item3}, 'c5e4623' used by {Item2, Item5}

Code

package tmp;

import static org.junit.Assert.assertEquals;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Map.Entry;
import java.util.stream.Collectors;

import org.junit.Test;

public class Snippet {
  private static String verifyItems(List<Item> items) {
    Map<String, List<Item>> map = new HashMap<>();
    for (Item item : items) {
      String key = item.getCode();
      List<Item> list;
      if (map.containsKey(key)) {
        list = map.get(key);
      }
      else {
        list = new ArrayList<>();
        map.put(key, list);
      }
      list.add(item);
    }
    boolean isFirst = true;
    StringBuilder sb = new StringBuilder();
    for (Entry<String, List<Item>> e : map.entrySet()) {
      if (e.getValue().size() > 1) {
        if (isFirst) {
          sb.append("Some of the items are sharing the same code: ");
        }
        else {
          sb.append(", ");
        }
        isFirst = false;
        sb.append("'" + e.getKey() + "' used by ");
        sb.append(e.getValue().stream().map(i -> i.getDescription())
            .collect(Collectors.joining(", ", "{", "}")));
      }
    }
    return sb.toString();
  }

  @Test
  public void testEmpty() throws Exception {
    assertEquals("", verifyItems(Collections.emptyList()));
  }

  @Test
  public void testOk() throws Exception {
    List<Item> items = Arrays.asList(
        new Item("baf18c4", "Item1"),
        new Item("c5e4623", "Item2"),
        new Item("bf1932f", "Item3"));
    assertEquals("", verifyItems(items));
  }

  @Test
  public void testDuplicate() throws Exception {
    List<Item> items = Arrays.asList(
        new Item("baf18c4", "Item1"),
        new Item("c5e4623", "Item2"),
        new Item("baf18c4", "Item3"),
        new Item("baf18c4", "Item4"));
    assertEquals("Some of the items are sharing the same code: 'baf18c4' used by {Item1, Item3, Item4}", verifyItems(items));
  }

  @Test
  public void testDuplicate2() throws Exception {
    List<Item> items = Arrays.asList(
        new Item("baf18c4", "Item1"),
        new Item("c5e4623", "Item2"),
        new Item("baf18c4", "Item3"),
        new Item("bf1932f", "Item4"),
        new Item("c5e4623", "Item5"));
    assertEquals("Some of the items are sharing the same code: 'baf18c4' used by {Item1, Item3}, 'c5e4623' used by {Item2, Item5}", verifyItems(items));
  }

  public static class Item {
    private String code;
    private String description;

    public Item(String code, String description) {
      super();
      this.code = code;
      this.description = description;
    }

    public String getCode() {
      return code;
    }

    public String getDescription() {
      return description;
    }
  }
}
\$\endgroup\$
  • 3
    \$\begingroup\$ The answer shows a better way. It is incredible what verbose code developers (including me) routinely wrote before the advent of Java streams and C# LINQ. \$\endgroup\$ – usr Aug 21 '17 at 13:15
21
\$\begingroup\$

Thank you for a nicely formulated and formatted first question! :)

Indeed, the same result can be achieved with much less code using Java 8.

I suggest the following:

  1. Group all the items into a Map<String, List<Item>>, equivalent of the first original for loop:

    Map<String, List<Item>> itemsGroupedByCode = 
      items.stream().collect(Collectors.groupingBy(Item::getCode));
    
  2. Collect all the messages citing duplicate entries, equivalent of the second original for loop, but without the prefix:

    String duplicatesMessage =
      itemsGroupedByCode.entrySet().stream()
          .filter(entry -> entry.getValue().size() > 1)
          .map(entry -> {
            final String duplicateValues = entry.getValue().stream()
                                                           .map(Item::getDescription)
                                                           .collect(Collectors.joining(", ", "{", "}"));
            return String.format("'%1$s' used by %2$s", entry.getKey(), duplicateValues);
      }).collect(Collectors.joining(", "));
    
  3. Return the result, empty if there were no duplicates or prefixed if there were some entries:

    if (!duplicatesMessage.isEmpty()) {
      return "Some of the items are sharing the same code: " + duplicatesMessage;
    }
    return "";
    

I'd also suggest to change the return type of the method to Optional<String> and return Optional.empty() instead of the empty string. But this is up to you to decide if it fits the context of your app.

\$\endgroup\$
  • \$\begingroup\$ ... and I'm marking that question as favorite because of your answer! (and I'm also crying because I don't understand the code you wrote just reading it. Need to learn about Java 8 streams first...). \$\endgroup\$ – Matthieu Aug 21 '17 at 17:45
  • 1
    \$\begingroup\$ You could make use of a custom Collector to do the second part a bit more neatly. In fact, you could probably do it in one loop with a Collector. For example using toMap with a downStream and a finisher. \$\endgroup\$ – Boris the Spider Aug 21 '17 at 18:40
  • \$\begingroup\$ @Matthieu: Thank you, I did not really expect this short one to receive so many upvotes :) Concerning Java 8, you have probably noticed that the functional style allows to significantly reduce boilerplate instructions. If you become familiar with the concepts of functional filtering, mapping and collecting, you'll be happy to use it and to leave the old style away ;) \$\endgroup\$ – Antot Aug 21 '17 at 18:48

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.