Converting array iteration to lambda function using Java8

Question

I am trying to convert to Lambda function

So far I am able to convert the above code to lambda function like as shown below

Stream.of(acceptedDetails, rejectedDetails)
.filter(list -> !isNull(list) && list.length > 0)
.forEach(new Consumer<Object>() {
    public void accept(Object acceptedOrRejected) {
        String id;
        if(acceptedOrRejected instanceof EmployeeValidationAccepted) {
            id = ((EmployeeValidationAccepted) acceptedOrRejected).getId();
        } else {
            id = ((EmployeeValidationRejected) acceptedOrRejected).getAd().getId();
        }

        if(acceptedOrRejected instanceof EmployeeValidationAccepted) {
            dates1.add(new Integer(id.split("something")[1]));
            Integer empId = Integer.valueOf(id.split("something")[2]);
            empIds1.add(empId);
        } else {
            dates2.add(new Integer(id.split("something")[1]));
            Integer empId = Integer.valueOf(id.split("something")[2]);
            empIds2.add(empId);
        }
    }
});

But still my goal was to avoid repeating the same logic and also to convert to Lambda function, still in my converted lambda function I feel its not clean and efficient.

This is just for my learning aspect I am doing this stuff by taking one existing code snippet.

Can anyone please tell me how can I improvise the converted Lambda function

This is quite the big example and the below code does instanceof checks which the imperative doesn't, what exactly is your goal? The use of instanceof seems like bad style to me — roookeee, May 27 '19 at 22:25
If this part of your application is not performance-sensitive (like really sensitive) I would do one stream.map() etc. for `empIdAccepted`, one for `dateAccepted`and so on as it is more clear and separates the logic you displayed into different aspects that are not related to one another. This is the foundation of functional programming: decomposing into sub-operations or steps — roookeee, May 27 '19 at 22:27
My goal is to convert to lambda function without any code or logic repeating — Alex Man, May 27 '19 at 22:30
Functional programming is partially about a more granular approach wherein you chunk / divide the work across multiple steps - repeating code and / or steps is a given when you stop implementing functions that do everything on their own (e.g. null-checking + conversion instead of one thing) — roookeee, May 27 '19 at 22:43
In your `filter` step, the stream elements seem to be arrays (you are accessing `list.length`), then, in the `forEach` step they are suddenly supposed to be instances of `EmployeeValidationAccepted` or `EmployeeValidationRejected`. This can’t work. It’s also not constructive to replace your working original code by this incomplete broken code. In the original code, you had `dateRejected` and `empIdRejected`, but now you have `dates1`, `dates2`, `empIds1`, and `empIds2` which are nowhere declared. — Holger, May 28 '19 at 08:57

score 4 · Answer 1 · answered May 28 '19 at 09:51

Generally, when you try to refactor code, you should only focus on the necessary changes.

Just because you’re going to use the Stream API, there is no reason to clutter the code with checks for null or empty arrays which weren’t in the loop based code. Neither should you change BigInteger to Integer.

Then, you have two different inputs and want to get distinct results from each of them, in other words, you have two entirely different operations. While it is reasonable to consider sharing common code between them, once you identified identical code, there is no sense in trying to express two entirely different operations as a single operation.

First, let’s see how we would do this for a traditional loop:

static void addToLists(String id, List<Integer> empIdList, List<BigInteger> dateList) {
    String[] array = id.split("-");
    dateList.add(new BigInteger(array[1]));
    empIdList.add(Integer.valueOf(array[2]));
}

List<Integer> empIdAccepted = new ArrayList<>();
List<BigInteger> dateAccepted = new ArrayList<>();

for(EmployeeValidationAccepted acceptedDetail : acceptedDetails) {
    addToLists(acceptedDetail.getId(), empIdAccepted, dateAccepted);
}

List<Integer> empIdRejected = new ArrayList<>();
List<BigInteger> dateRejected = new ArrayList<>();

for(EmployeeValidationRejected rejectedDetail : rejectedDetails) {
    addToLists(rejectedDetail.getAd().getId(), empIdRejected, dateRejected);
}

If we want to express the same as Stream operations, there’s the obstacle of having two results per operation. It truly took until JDK 12 to get a built-in solution:

static Collector<String,?,Map.Entry<List<Integer>,List<BigInteger>>> idAndDate() {
    return Collectors.mapping(s -> s.split("-"),
        Collectors.teeing(
            Collectors.mapping(a -> Integer.valueOf(a[2]), Collectors.toList()),
            Collectors.mapping(a -> new BigInteger(a[1]),  Collectors.toList()),
            Map::entry));
}

Map.Entry<List<Integer>, List<BigInteger>> e;
e = Arrays.stream(acceptedDetails)
        .map(EmployeeValidationAccepted::getId)
        .collect(idAndDate());

List<Integer> empIdAccepted = e.getKey();
List<BigInteger> dateAccepted = e.getValue();

e = Arrays.stream(rejectedDetails)
    .map(r -> r.getAd().getId())
    .collect(idAndDate());

List<Integer> empIdRejected = e.getKey();
List<BigInteger> dateRejected = e.getValue();

Since a method can’t return two values, this uses a Map.Entry to hold them.

To use this solution with Java versions before JDK 12, you can use the implementation posted at the end of this answer. You’d also have to replace Map::entry with AbstractMap.SimpleImmutableEntry::new then.

Or you use a custom collector written for this specific operation:

static Collector<String,?,Map.Entry<List<Integer>,List<BigInteger>>> idAndDate() {
    return Collector.of(
        () -> new AbstractMap.SimpleImmutableEntry<>(new ArrayList<>(), new ArrayList<>()),
        (e,id) -> {
            String[] array = id.split("-");
            e.getValue().add(new BigInteger(array[1]));
            e.getKey().add(Integer.valueOf(array[2]));
        },
        (e1, e2) -> {
            e1.getKey().addAll(e2.getKey());
            e1.getValue().addAll(e2.getValue());
            return e1;
        });
}

In other words, using the Stream API does not always make the code simpler.

As a final note, we don’t need to use the Stream API to utilize lambda expressions. We can also use them to move the loop into the common code.

static <T> void addToLists(T[] elements, Function<T,String> tToId,
                           List<Integer> empIdList, List<BigInteger> dateList) {
    for(T t: elements) {
        String[] array = tToId.apply(t).split("-");
        dateList.add(new BigInteger(array[1]));
        empIdList.add(Integer.valueOf(array[2]));
    }
}

List<Integer> empIdAccepted = new ArrayList<>();
List<BigInteger> dateAccepted = new ArrayList<>();
addToLists(acceptedDetails, EmployeeValidationAccepted::getId, empIdAccepted, dateAccepted);

List<Integer> empIdRejected = new ArrayList<>();
List<BigInteger> dateRejected = new ArrayList<>();
addToLists(rejectedDetails, r -> r.getAd().getId(), empIdRejected, dateRejected);

score 2 · Accepted Answer · answered May 28 '19 at 03:53

A similar approach as @roookeee already posted with but maybe slightly more concise would be to store the mappings using mapping functions declared as :

Function<String, Integer> extractEmployeeId = empId -> Integer.valueOf(empId.split("-")[2]);
Function<String, BigInteger> extractDate = empId -> new BigInteger(empId.split("-")[1]);

then proceed with mapping as:

Map<Integer, BigInteger> acceptedDetailMapping = Arrays.stream(acceptedDetails)
        .collect(Collectors.toMap(a -> extractEmployeeId.apply(a.getId()),
                a -> extractDate.apply(a.getId())));

Map<Integer, BigInteger> rejectedDetailMapping = Arrays.stream(rejectedDetails)
        .collect(Collectors.toMap(a -> extractEmployeeId.apply(a.getAd().getId()),
                a -> extractDate.apply(a.getAd().getId())));

Hereafter you can also access the date of acceptance or rejection corresponding to the employeeId of the employee as well.

Another good idea! My answer could be extended with `Stream.concat` instead of collecting both variants and then filtered like you described too — roookeee, May 28 '19 at 09:29
This assumes that the ids are truly unique and that the order doesn’t matter. Further, it’s performing `split` twice per element. — Holger, May 28 '19 at 09:53

score 1 · Answer 3 · answered May 27 '19 at 22:37

How about this:

 class EmployeeValidationResult {
    //constructor + getters omitted for brevity
    private final BigInteger date;
    private final Integer employeeId;
}

List<EmployeeValidationResult> accepted = Stream.of(acceptedDetails)
    .filter(Objects:nonNull)
    .map(this::extractValidationResult)
    .collect(Collectors.toList());

List<EmployeeValidationResult> rejected = Stream.of(rejectedDetails)
    .filter(Objects:nonNull)
    .map(this::extractValidationResult)
    .collect(Collectors.toList());


EmployeeValidationResult extractValidationResult(EmployeeValidationAccepted accepted) {
    return extractValidationResult(accepted.getId());
}

EmployeeValidationResult extractValidationResult(EmployeeValidationRejected rejected) {
    return extractValidationResult(rejected.getAd().getId());
}

EmployeeValidationResult extractValidationResult(String id) {
    String[] empIdList = id.split("-");
    BigInteger date = extractDate(empIdList[1])
    Integer empId = extractId(empIdList[2]);

    return new EmployeeValidationResult(date, employeeId);
}

Repeating the filter or map operations is good style and explicit about what is happening. Merging the two lists of objects into one and using instanceof clutters the implementation and makes it less readable / maintainable.

Converting array iteration to lambda function using Java8

3 Answers3

Linked