How to compute Map from stream, to then check property of map values?

Question

My requirement: I have an interface that shall only contain entries such as public final static short SOME_CONST = whatever. The catch: the short constants need to be unique. And in when there are duplicates, I am mainly interested in having the SOME_CONST_A, SOME_CONST_B, ... names causing the conflict.

I wrote the below test to test that via reflection. It works, but I find it clunky and not very elegant:

@Test
public void testIdsAreUnique() {
    Map<Short, List<String>> fieldNamesById = new LinkedHashMap<>();
    Arrays.stream(InterfaceWithIds.class.getDeclaredFields())
            .filter(f -> f.getClass().equals(Short.class))
            .forEach((f) -> {
        Short key = null;
        String name = null;
        try {
            key = f.getShort(null);
            name = f.getName();
        } catch (IllegalAccessException e) {
            throw new RuntimeException(e);
        }
        fieldNamesById.computeIfAbsent(key, x -> new ArrayList<>()).add(name);
    });

    assertThat(fieldNamesById.entrySet().stream().filter(e -> e.getValue().size() > 1)
            .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue)), is(Collections.emptyMap()));
}

Is there a way to avoid that intermediate local map instance?

( bonus question: is there a nicer way to shorten the lambda that fills the map with key/value pairs? )

Answer 1

If you want this check efficiently (normally not so much a concern for unit test), you can reduce the work by optimistically assuming that the fields have no duplicates and performing a cheap pre-test first. Further, you can use the result of this pre-test for getting the actual field with duplicates (if there are some) without a Map.

As pre-requisite, we should encapsulate the reflective operation

private static int fieldValue(Field f) {
    try {
        return f.getShort(null);
    }
    catch(ReflectiveOperationException ex) {
        throw new IllegalStateException();
    }
}

Further, we need to map the potential values of the short value range to a positive index for a BitSet:

private static int shortToIndex(int shortValue) {
    return Math.abs(shortValue<<1) | (shortValue>>>31);
}

This assumes that numbers with smaller magnitude are more common and keeps their magnitude small, to reduce the size of the resulting BitSet. If the values are assumed to be positive, shortValue & 0xffff would be preferable. If neither applies, you could also use shortValue - Short.MIN_VALUE instead.

Having the mapping function, we can use

@Test
public void testIdsAreUnique() {
    BitSet value = new BitSet(), duplicate = new BitSet();

    Field[] fields = InterfaceWithIds.class.getDeclaredFields();
    Arrays.stream(fields)
        .filter(f -> f.getType() == short.class)
        .mapToInt(f -> shortToIndex(fieldValue(f)))
        .forEach(ix -> (value.get(ix)? duplicate: value).set(ix));

    if(duplicate.isEmpty()) return; // no duplicates

    throw new AssertionError(Arrays.stream(fields)
        .filter(f -> duplicate.get(shortToIndex(fieldValue(f))))
        .map(f -> f.getName()+"="+fieldValue(f))
        .collect(Collectors.joining(", ", "fields with duplicate values: ", "")));
}

It first fills a bitset for all encountered values and another bitset for those encountered more than once. If the latter bitset is empty, we can return immediately as there are no duplicates. Otherwise, we can use that bitset as a cheap filter to get the field having the problematic values.

Answer 2

With your actual solution you are not very far.
You could relying on groupingBy() and mapping() in a first map collect in order to collect field names by field value. In this way you don't need any intermediary Map.

Map<Short, List<String>> map = 
Arrays.stream(InterfaceWithIds.class.getDeclaredFields())
     .filter(f -> f.getType()
                   .getClass()
                   .equals(short.class))
     .map(f -> {
         Short key = null;
         String name = null;
         try {
             key = f.getShort(null);
             name = f.getName();
         } catch (IllegalAccessException e) {
             throw new RuntimeException(e);
         }
         return new AbstractMap.SimpleEntry<>(key, name);
     })
     .collect(groupingBy(SimpleEntry::getKey, LinkedHashMap::new, mapping(e -> e.getValue(), Collectors.toList())))
     .entrySet()
     .stream()
     .filter(e -> e.getValue()
                   .size() > 1)
     .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));

assertThat(map, is(Collections.emptyMap()));

Answer 3

Here's a stream that's grouping fields by the static value. Note some comments about other changes/corrections

Map<Short, List<String>> fieldNamesById = 
        Arrays.stream(InterfaceWithIds.class.getDeclaredFields())

         //using short.class, not Short.class
        .filter(f -> f.getType().equals(short.class)) 

        //group by value, mapping fields to their names in a list
        .collect(Collectors.groupingBy(f -> getValue(f),
                Collectors.mapping(Field::getName, Collectors.toList())));

The method called to read the value is below (primarily meant to avoid try/catch blocks in the stream):

private static Short getValue(Field f) {
    try {
        return f.getShort(null);
    } catch (Exception e) {
        throw new RuntimeException(e);
    }
}

Answer 4

Not sure this would fit your needs, but why not simply:

 ...filter(..)
    .collect(Collectors.toMap(f -> f.getShort(null), Field::getName))

If there are duplicates, this will fail with an Exception. Catch that and do Assert.fail(...) for example.

I hope I got the code right, typing this on the phone

Answer 5

A few problems here. Firstly f.getClass() will give you the class of the Field instance, and not the actual class of the field. You want

f.getType().equals(Short.class)

Next, you need to remember that Short.class and short.class are different, so you actually want

f.getType().equals(Short.class) || f.getType().equals(short.class)

I would personally take advantage of that fact that map.put returns the previous value for the given key. Because we hope that there will never have been a previous value, we can simply call assertNull on the result.

Your entire test would look like this:

Map<Short, String> fieldNamesById = new LinkedHashMap<>();
Arrays.stream(InterfaceWithIds.class.getDeclaredFields())
    .filter(f -> f.getType().equals(Short.class) || f.getType().equals(short.class))
    .forEach((f) -> {
        Short key = null;
        String name = null;
        try {
            key = f.getShort(null);
            name = f.getName();
        } catch (IllegalAccessException e) {
            throw new RuntimeException(e);
        }

        assertNull(fieldNamesById.put(key, name));
    });

If you want to report all errors, try this:

List<String> problems = new ArrayList<>();

Map<Short, String> fieldNamesById = new LinkedHashMap<>();
Arrays.stream(InterfaceWithIds.class.getDeclaredFields())
    .filter(f -> f.getType().equals(Short.class) || f.getType().equals(short.class))
    .forEach((f) -> {
        Short key = null;
        String name = null;
        try {
            key = f.getShort(null);
            name = f.getName();
        } catch (IllegalAccessException e) {
            throw new RuntimeException(e);
        }

        String prevValue = fieldNamesById.put(key, name);
        if (prevValue != null) problems.add("key " + key + " mapped to " + name + " and " + prevValue);
    });

assertTrue(problems.toString(), problems.isEmpty());