CODEWITHSUNDEEP
javajava-stream
Boris Parnikel
Boris Parnikel

Reputation: 863

Java Stream API with non-pure functions chain

Let's assume I have a class Person

public class Person {
    private final String name;
    private final int age;
    private boolean rejected;
    private String rejectionComment;

    public void reject(String comment) {
        this.rejected = true;
        this.rejectionComment = comment;
    }

    // constructor & getters are ommited
}

and my app is something like that

class App {
    public static void main(String[] args) {
        List<Person> persons = Arrays.asList(
            new Person("John", 10),
            new Person("Sarah", 20),
            new Person("Daniel", 30)
        )

        persons.forEach(p -> {
            rejectIfYoungerThan15(p);
            rejectIfNameStartsWithD(p);
            // other rejection functions
        }
    }

    private static void rejectIfYoungerThan15(Person p) {
        if (!p.isRejected() && p.getAge() < 15) {
            p.reject("Too young")
        }
    }

    private static void rejectIfNameStartsWithD(Person p) {
        if (!p.isRejected() && p.getName().startsWith("D")) {
            p.reject("Name starts with 'D'")
        }
    }

    // other rejection functions
}

The thing is I don't like that I have to perform !p.isRejected() check in every rejection function. Moreover, it doesn't make sense to pass an already rejected person to next filters. So my idea is to use a mechanism of Stream.filter and make something like

persons.stream().filter(this::rejectIfYoungerThan15).filter(this::rejectIfNameStartsWithD)...

And change signature for these methods to return true if a passed Person has not been rejected and false otherwise.

But it seems to me that it's a very bad idea to use filter with non-pure functions.

Do you have any ideas of how to make it in more elegant way?

Upvotes: 0

Views: 371

Answers (3)

Holger
Holger

Reputation: 298203

When you change the check functions to only check the condition (i.e. not to call p.isRejected()) and return boolean, you already made the necessary steps to short-circuit:

private static boolean rejectIfYoungerThan15(Person p) {
    if(p.getAge() < 15) {
        p.reject("Too young");
        return true;
    }
    return false;
}

private static boolean rejectIfNameStartsWithD(Person p) {
    if(p.getName().startsWith("D")) {
        p.reject("Name starts with 'D'");
        return true;
    }
    return false;
}

usable as

   persons.forEach(p -> {
        if(rejectIfYoungerThan15(p)) return;
        if(rejectIfNameStartsWithD(p)) return;
        // other rejection functions
    }
}

A Stream’s filter operation wouldn’t do anything other than checking the returned boolean value and bail out. But depending on the Stream’s actual terminal operation the short-circuiting could go even farther and end up in not checking all elements, so you should not bring in a Stream operation here.

Upvotes: 1

marstran
marstran

Reputation: 28036

You should make Person immutable, and let the reject-methods return a new Person. That will allow you to chain map-calls. Something like this:

public class Person {
    private final String name;
    private final int age;
    private final boolean rejected;
    private final String rejectionComment;

    public Person reject(String comment) {
        return new Person(name, age, true, comment);
    }

    // ...

}

class App {

    // ...

    private static Person rejectIfYoungerThan15(Person p) {
        if (!p.isRejected() && p.getAge() < 15) {
            return p.reject("Too young");
        }
        return p;
    }
}

Now you can do this:

persons.stream()
       .map(App::rejectIfYoungerThan15)
       .map(App::rejectIfNameStartsWithD)
       .collect(Collectors.toList());

If you want to remove rejected persons, you can add a filter after the mapping:

.filter(person -> !person.isRejected())

EDIT:

If you need to short circuit the rejections, you could compose your rejection functions into a new function and make it stop after the first rejection. Something like this:

/* Remember that the stream is lazy, so it will only call new rejections 
 * while the person isn't rejected.
 */
public Function<Person, Person> shortCircuitReject(List<Function<Person, Person>> rejections) {
    return person -> rejections.stream()
            .map(rejection -> rejection.apply(person))
            .filter(Person::isRejected)
            .findFirst()
            .orElse(person);
}

Now your stream can look like this:

List<Function<Person, Person>> rejections = Arrays.asList(
    App::rejectIfYoungerThan15, 
    App::rejectIfNameStartsWithD);

List<Person> persons1 = persons.stream()
    .map(shortCircuitReject(rejections))
    .collect(Collectors.toList());

Upvotes: 0

Darshan Mehta
Darshan Mehta

Reputation: 30819

Calling these methods from lambda is fine, however, for better readability, you can rename these methods to show what they are doing and return boolean, e.g.:

private boolean hasEligibleAge(Person p){..}
private boolean hasValidName(Person p){..}

Another approach would be to wrap these methods into another method (to reflect the business logic/flow), e.g.:

private boolean isEligible(Person p){
    //check age
    //check name
}

Upvotes: 0

Related Questions