Mapping classes with lambda to default value

Follow up question from this.
Having a hierarchy like this. Where the A is the base class:

       A 
      / /
     B   C

  |   A    |   |   B       |   |  C      |  
  | getId()|   |A.getId()  |   |A.getId()|
               |isVisible()| 

and the following content:

List<A> mappings;

I would like to map all IDs of instances of B to the value of B.isVisible() and the IDs of instances of C to TRUE

With the help of the initial question, I refined it to this format:

mappings.stream().filter(a -> a instanceof B)
                 .map(b -> (B)b)
                 .collect(Collectors.toMap(A::getId, m -> m.isVisible()));

The ugly version is:

mappings.stream()                       
        .collect(Collectors.toMap(A::getId, m ->
                        {
                            boolean isB = m instanceof B;
                            return isB ? ((B) m).isVisible() : true;
                        }));

Any help on improving it to provide the default true for the more elegant version?

Your variant

mappings.stream()                       
        .collect(Collectors.toMap(A::getId, m ->
                        {
                            boolean isB = m instanceof B;
                            return isB ? ((B) m).isVisible() : true;
                        }));

isn’t that ugly, as it expresses your intention.

But you can simplify it, as you don’t need a local variable to hold m instanceof B:

mappings.stream()                       
        .collect(Collectors.toMap(A::getId, m->m instanceof B? ((B)m).isVisible(): true));

Then, as a rule of thumb, whenever you have a boolean literal in a compound boolean expression, there is an alternative without it.

mappings.stream()                       
        .collect(Collectors.toMap(A::getId, m -> !(m instanceof B) || ((B)m).isVisible()));

Your code is ugly because your hierarchy doesn’t make sense. What you probably want is something like:

class A
{
    abstract public boolean isVisible();
    // or make it concrete and return a default if you need to
}

// B can stay the same (+ @Override)

class C extends A
{
    @Override
    public boolean isVisible()
    {
        return true;
    }
}

Then can just do:

mappings.stream()
        .collect(Collectors.toMap(A::getId, m -> m.isVisible()));

I’ve written this simple Collector implementation which should do what you want:

public class AToMapCollector implements Collector<A, Map<Integer, Boolean>, Map<Integer, Boolean>>{
    @Override
    public Supplier<Map<Integer, Boolean>> supplier(){
        return HashMap::new;
    }

    @Override
    public BiConsumer<Map<Integer, Boolean>, A> accumulator(){
        return (map, a) -> {
            boolean visible = true;
            if(a instanceof B){
                visible = ((B) a).isVisible();
            }
            map.put(a.getId(), visible);
        };
    }

    @Override
    public BinaryOperator<Map<Integer, Boolean>> combiner(){
        return (map1, map2) -> {
            map1.putAll(map2);
            return map1;
        };
    }

    @Override
    public Function<Map<Integer, Boolean>, Map<Integer, Boolean>> finisher(){
        return map -> map;
    }

    @Override
    public Set<Characteristics> characteristics(){
        return EnumSet.of(Characteristics.IDENTITY_FINISH, Characteristics.UNORDERED);
    }
}

which then finally can be called like this:

Map<Integer, Boolean> map = mappings.stream().collect(new AToMapCollector());

The addition of creating a whole new class is the reusability of this collector and also increases the readability instead of a multiline lambda.

Maybe you can do what you want with a helper class:

class Helper {
    private final Long id;
    private final boolean visible;

    Helper(A a) {
        this.id = a.getID();
        this.visible = a instanceof B ? ((B) a).isVisible() : true;
    }

    Long getId() { return id; }

    boolean isVisible() { return visible; }
}

Then, map each element of the list to an instance of Helper and collect to the map:

Map<Long, Boolean> map = mappings.stream()
    .map(Helper::new)
    .collect(Collectors.toMap(Helper::getId, Helper::isVisible));

This solution just delegates whether visible is true or false to the Helper class and lets you have a clean stream pipeline.

As a side note… In general, having a map with values of type Boolean is pointless, because you can have the same semantics with a Set:

Set<Long> set = mappings.stream()
    .map(Helper::new)
    .filter(Helper::isVisible)
    .collect(Collectors.toSet());

Then, to know if some element is visible or not, simply check whether it belongs to the set:

boolean isVisible = set.contains(elementId);

May be mapping C’s to null in the stream and then return true on null? Like this:

mappings.stream().map(a -> a instanceof C ? (B)null : (B)a)
                 .collect(Collectors.toMap(A::getId, m==null || m.isVisible()));

If you can’t change the source code, you can write an utility method isA to describe what you want, for example:

Map<Integer, Boolean> visibility = mappings.stream().collect(toMap(
        A::getId,
        isA(B.class, B::isVisible, any -> true)
));

static <T, S extends T, R> Function<T, R> isA(Class<? extends S> type,
                                              Function<? super S, R> special,
                                              Function<T, R> general) {

    return it -> type.isInstance(it) ? special.apply(type.cast(it))
                                     : general.apply(it);
}