3

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

5
  • 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 Commented May 27, 2019 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 dateAcceptedand 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 Commented May 27, 2019 at 22:27
  • My goal is to convert to lambda function without any code or logic repeating Commented May 27, 2019 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) Commented May 27, 2019 at 22:43
  • 1
    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. Commented May 28, 2019 at 8:57

3 Answers 3

4

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);
Sign up to request clarification or add additional context in comments.

Comments

2

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.

2 Comments

Another good idea! My answer could be extended with Stream.concat instead of collecting both variants and then filtered like you described too
This assumes that the ids are truly unique and that the order doesn’t matter. Further, it’s performing split twice per element.
1

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.

Comments

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.