1

I need to refactor my below stream code:

    List<Map<String, Integer>> list5 = new ArrayList<>();
    map3 = new HashMap<>();
    map3.put("foo", 1);
    map3.put("bar", 2);
    map3.put("zzz", 6);
    list5.add(map3);
    map3 = new HashMap<>();
    map3.put("foo", 3);
    map3.put("bar", 4);
    map3.put("zzz", null);
    list5.add(map3);

    //new list with processed maps
    List<Map<String, Integer>> list6 = list5.stream()
            .map(hashmap -> {
                Map<String, Integer> newMap = hashmap.entrySet().stream()
                        .collect(HashMap::new, (m, v) -> {
                            if (v.getKey().equals("bar")) {
                                m.put(v.getKey(), v.getValue() * 2);
                            } else {
                                m.put(v.getKey(), v.getValue());
                            }
                        }, HashMap::putAll);
                return newMap;
            })
            .collect(toList());
    System.out.println(list6);

I need a way to extract/refactor the below logic only from the above stream code, since this piece will only change in other list of maps that I have:

if (v.getKey().equals("bar")) {
    m.put(v.getKey(), v.getValue() * 2);
} else {
    m.put(v.getKey(), v.getValue());
}

Using IntelliJ it adds a biconsumer to main() itself, which is not expected here and removes the code. I need a way to extract it separately something like below:

List<Map<String, Integer>> list6 = list5.stream()
            .map(hashmap -> {
                Map<String, Integer> newMap = hashmap.entrySet().stream()
                        .collect(HashMap::new, (m, v) -> {
                            biconsumerLogic1.accept(m, v);
                        }, HashMap::putAll);
                return newMap;
            })
            .collect(toList());

And biconsumerLogic1 is a separate functional interface like:

BiConsumer biconsumerLogic1() {
    accept(m, v) {
         //logic goes here...
    }
}

How do I achieve that? Any pointers are appreciated.

Thanks..

Improve this question
3
  • It would be better if you tell what you are trying to do here. may be someone has a better solution than refactoring your code Commented Feb 1, 2018 at 15:27
  • Please see my comment below @Bohemian Commented Feb 1, 2018 at 15:37
  • Yes I will be doing that.. Thanks for reminding though.. SO does not allow me today, I guess I can't except my own answer within 24 hrs. Commented Feb 2, 2018 at 3:42

4 Answers 4

Reset to default
3

See, the problem is with using streams at all. You have overdesigned it, you don't need most of it.

Look:

List<Map<String, Integer>> newList = new ArrayList<>(list);

newList.replaceAll(eachMap -> {
  Map<String, Integer> map = new HashMap<>(eachMap);
  map.computeIfPresent("bar", (k,v) -> v * 2);
  return map;
});

And to replace the action, you need to do something like this:

BiFunction<String, Integer, Integer> action = (k,v) -> v * 2;

newList.replaceAll(eachMap -> {
  Map<String, Integer> map = new HashMap<>(eachMap);
  map.computeIfPresent("bar", action);
  return map;
});

Now you can extract the whole thing into a separate method and accept list and action (possibly the "bar" variable as well) as arguments, and here you have your value replacer.

Improve this answer
Sign up to request clarification or add additional context in comments.

17 Comments

You have simplified the operation to a degree that makes abstracting the modification unnecessary.
It’s not clear whether the OP wants to abstract the modification function only or both, the key selection and the modification (as the original function was the entire if (v.getKey().equals("bar")) { m.put(v.getKey(), v.getValue() * 2); } else { m.put(v.getKey(), v.getValue()); }). In the latter case, your operation merely consists of exactly that function. Ok, plus the map duplication, but the abstraction itself would also add another declaration, so it’s not simpler than just writing a single function. But let the OP decide…
@Holger, The map duplication came from original question. I don't know it it's required, but the piece did it, so I did too. As for the operation, it really can go either way. If we have a requirement to do both single key replacement and occasionally replace two keys at once, we can expose a Consumer<Map<String, Integer>> for map variable.
@M.Prokhorov It does looks clean, but i see few code smell here. 1> You iterate the list twice, which is bad for large data set. Once when you create a new list, secondly when you are doing replace all. So effectively your solution is doing 2x operations, whereas my solution though hard to read still manages everything in just 1 iteration of list through streams.
@Eugene a general solution would require a different interface. But for a lot of cases, e.g. when combining collectors, there wouldn’t be a useful hint anyway. In principle, there could be an internal tweak, when the builtin toList() returns a collector implementation which the builtin Stream.collect implementation recognizes.
|
2

I think there are better ways to do what you're actually doing. But strictly answering how to extract and refactor the indicated piece of code, you could do something like this:

static <K, V> List<Map<K, V>> process(
        List<Map<K, V>> source,
        BiConsumer<? super Map<K, V>, ? super Map.Entry<K, V>> accumulator) {

    return source.stream()
        .map(m -> m.entrySet().stream()
            .collect(HashMap::new, accumulator, Map::putAll))
        .collect(Collectors.toList());
}

Usage:

List<Map<String, Integer>> list6 = process(
    list5,
    (m, e) -> m.put(
        e.getKey(), 
        e.getKey().equals("bar") ? e.getValue() * 2 : e.getValue()));
Improve this answer

Comments

2

Below is my version of the refactoring using BiConsumers:

psv main(...) {
    List<Map<String, Integer>> newList = processMapData(origList, accumulator());
    System.out.println(newList);
}

private static List<Map<String, Integer>> processMapData(List<Map<String, Integer>> origList, BiConsumer<HashMap<String, Integer>, Map.Entry<String, Integer>> accumulator) {
    return origList.stream()
                .map(hashmap -> {
                    Map<String, Integer> newMap = hashmap.entrySet().stream()
                            .collect(HashMap::new, accumulator, HashMap::putAll);
                    return newMap;
                })
                .collect(toList());
}

private static BiConsumer<HashMap<String, Integer>, Map.Entry<String, Integer>> accumulator() {
    return (m, v) -> {
        m.put(v.getKey(), v.getKey().equals("bar") ? v.getValue() * 2: v.getValue());
    };
}

I could leverage more control and dynamically inject accumulators to processMapData function. Thanks to @Federico for the BiConsumer function suggestion.

Improve this answer

Comments

1

How about:

list5.stream().forEach(map -> map.replaceAll((k,v) -> k.equals("bar") ? v * 2 : v));

This does an in-place replacement of values.

If you absolutely need a new map:

list6 = list5.stream()
    .map(m -> m.entrySet().stream()
        .collect(toMap(Map.Entry::getKey, e -> e.getValue() * (e.getKey().equals("bar") ?  2 : 1))))
    .collect(toList());

If you're looking to refactor, there are two options:

Option 1:

Create a BiFunction, eg:

public static BiFunction<String, Integer, Integer> valueCalculator() {
    return (k,v) -> k.equals("bar") ? v * 2 : v;
}

and to use:

list5.stream().forEach(map -> map.replaceAll(valueCalculator()));

Option 2:

Create a method:

public Integer valueCalculator(String key, Integer value) {
    return key.equals("bar") ? value * 2 : value;
}

and to use, use a method reference:

list5.stream().forEach(map -> map.replaceAll(this::valueCalculator));

of if it's a static method:

list5.stream().forEach(map -> map.replaceAll(MyClass::valueCalculator));

I prefer option 2, because it's easier understand, test and debug.

Improve this answer

5 Comments

Nope. My outer stream code is going to remain same. Only part that is going to change is the if () {} logic. Here I am changing values by doing multiplication, but I may have a logic to do something else. SO I want to inject that logic dynamically. and I can have several such scenarios.
@GauravSaini I've added some options of how to refactor
replaceAll is a big overkill when you only concerned with changing the value of a single key. This is what computeIfPresent is for.
@M.Prokhorov sure it's overkill, but it's not a problem if the map is small. Programmer productivity counts too.
@Bohemian, I would argue that there is no productivity gain in saying "replace everything here" when in reality you only actuall want the single thing changed, and everything else to stay the same. Unless we do want to leave the ability to change multiple keys at once, using All would pollute the intention. I know I would be confused when I came across this. And the next guy, and the next. This means gaining a little for one person, and losing some for the rest (and you yourself is a different person the next day when you read what you wrote the day before).

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.