Mapping classes with lambda to default value

Question

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 examples are little confusing. In your last lambda you named variable `m` but you are using `mapping instanceof B`. Please check your examples to avoid problems which are unrelated to your question. Also `.filter(a -> instanceof B)` should probably be `.filter(a -> a instanceof B)` — Pshemo, Jul 27 '17 at 13:24
thanks @Eugene. I also edited it to correct the issues that I've missed. I hope it's clearer now. — Olimpiu POP, Jul 27 '17 at 13:30
You should be able to reduce `m -> { boolean isB = m instanceof B; return isB ? ((B) m).isVisible() : true; }` to `m -> m instanceof B ? ((B) m).isVisible() : true` (although it is still unclear if that is what you wanted). — Pshemo, Jul 27 '17 at 13:32
seriously - what you have in place is not ugly. you will not get a better advice. Pshemo is right - this is the only thing you should do to make your code better... — Eugene, Jul 27 '17 at 13:43

score 7 · Accepted Answer · answered Jul 27 '17 at 17:38

7

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()));

answered Jul 27 '17 at 17:38

Holger

285,553
42
434
765

2

@Eugene: maybe it’s worth posting a full table of all combinations for demonstration… – Holger Jul 28 '17 at 10:32
3

@Eugene: nope, unnecessarily complex boolean expressions are a recurring thing, not only on Stackoverflow, so I think, such a table, reminding developers of the simpler alternatives and proving that my ad hoc rule works, i.e. that it should always ring a bell if you see `true` or `false` in any other context than as a sole constant, could have some worth. – Holger Jul 28 '17 at 10:37
1

ok, I do agree. But that should be a separate Q/A... If you post such, I will be more then happy to up vote - most probably even import it in our code base – Eugene Jul 28 '17 at 10:40
@Holger If I were to implement an alert for i.e. Sonar or FindBugs based on your rule of thumb, I would also consider constant predicates valid, i.e. `t -> true`, I mean, apart from `boolean variable = true`. – fps Jul 30 '17 at 01:57
1

@Federico Peralta Schaffner: that’s why I said “whenever you have a boolean literal in a ***compound*** boolean expression”. A sole literal doesn’t count. The simplification rules may even evaluate to a boolean literal, e.g. `a && false` → `false`… – Holger Jul 31 '17 at 08:47

fps · Answer 2 · 2017-07-27T20:29:48.273

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);

first, up! we are the same. but I used an utility method instead. — holi-java, Jul 27 '17 at 16:56

score 3 · Answer 3 · answered Jul 27 '17 at 16:56

3

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);
}

answered Jul 27 '17 at 16:56

holi-java

29,655
7
72
83

Hi, Liang! Nice approach, very functional – fps Jul 27 '17 at 17:15
1

@FedericoPeraltaSchaffner thanks, inspired by your answer. :) – holi-java Jul 27 '17 at 17:16
@FedericoPeraltaSchaffner Your memory is very good. thanks for remember me, my friend. :) – holi-java Jul 27 '17 at 17:18
please accept FedericoPeraltaSchaffner's answer, my answer inspired from his. – holi-java Jul 27 '17 at 17:21
but yours is much better, doesn't have not needed helper class – fps Jul 27 '17 at 17:22
@FedericoPeraltaSchaffner sir, because I steal your thought. this is why I say we are the same in you answer comment. – holi-java Jul 27 '17 at 17:23
Let us [continue this discussion in chat](http://chat.stackoverflow.com/rooms/150342/discussion-between-holi-java-and-federico-peralta-schaffner). – holi-java Jul 27 '17 at 17:25

score 2 · Answer 4 · answered Jul 27 '17 at 13:26

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()));

yes, I thought this as well, but I can't change the hierarchy. Thanks — Olimpiu POP, Jul 27 '17 at 13:46

Jean-Baptiste Yunès · Answer 5 · 2017-07-28T06:38:55.417

1

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()));

edited Jul 28 '17 at 06:38

answered Jul 27 '17 at 15:54

Jean-Baptiste Yunès

34,548
4
48
69

2

`m==null || m.isVisible()` – Holger Jul 27 '17 at 17:39
`.map(a -> a instanceof C ? (B)null : (B)b)` should be `.map(a -> a instanceof B ? (B)a : (B)null)` or am i wrong? – Lino Jul 28 '17 at 05:55

Lino · Answer 6 · 2017-07-27T13:45:05.150

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.

and also how is this better then a *much* simpler lambda expression? I fail to see the benefit of this... — Eugene, Jul 27 '17 at 13:40
i did go out from the original question, not from the comments which now provide a simple lambda expression — Lino, Jul 27 '17 at 13:45

Mapping classes with lambda to default value

6 Answers6