3

I have few person objects and a function to select the person for the given entity id:

person1 = {
    name = "abc";
    id = "1";
}
person2 = {
    name = "xyz";
    id = "2";
}
person3 = {
    name = "aaa";
    id = null;
}

Person getPersonFunction(List<Person> persons, String id) {
   //Get the person for the given id and if id is not for any person from the list then select the person with null as id.
}

Here is the problem: I want to select the person for a given entity id and if there is no such person present in the list for the given entity id then select the person whose entity id is set to null. These will be always one person whose entity id is set to null and it's like a default person. I can solve this problem with easily by using for loop but I want to solve this problem using java stream. Here is what I did:

Map<String, Person> personMap = persons.stream().collect(Collectors.toMap(Person::id, person -> person);

Person person = personMap.getOrDefault(id, personMap.get(null))

This gives me the correct result but I want to do it in a single line.

Improve this question
5
  • 7
    Why does it need to be on a single line? Commented Mar 23, 2018 at 9:52
  • Delete the line break and voilá - single line! Commented Mar 23, 2018 at 9:54
  • 3
    Why do you build a map if you search for a single id? Just search linearly. Commented Mar 23, 2018 at 9:59
  • Are you sure a List is the right data structure to use in the first place? If ids are unique it might be better to use a Map throughout. Commented Mar 23, 2018 at 10:04
  • @rustyx he needs it to kind of optimizing his search. First he will search the whole List in O(n) steps. Then in case he doesn't find its element he will use the map for kind of O(1) steps to find the element with id==null Commented Mar 23, 2018 at 10:07

4 Answers 4

Reset to default
3

I would recommend avoiding trying to eliminate lines by use of the Stream API; that is not its purpose.

Your idea of processing the collection and building a Map is a good one. You may also be able to reuse the map, which would not be possible if you tried to force this into one line. In addition, the clear divide between building the Map then using it to pull the Person that satisfies your constraints is self-documenting (it makes it easier for people to understand your code).

Think more about readability and algorithm design. Think less about how to accomplish tasks with the smallest line count.

This is of course not to say that you should not reduce code duplication.

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

Comments

3

The crystal clear way of doing this would be to 

Person NOBODY = new Person("NOBODY", null);

private Person findPerson(List<Person> people, String id) {
    return people.stream()
            // Anyone with the right or null id.
            .filter(p -> p.id == null || p.id.equals(id))
            // Sort putting null ids at the end.
            .sorted(Comparator.comparing(p -> p.id == null))
            // Take first.
            .findFirst()
            // Should never happen.
            .orElse(NOBODY);
}
Improve this answer

6 Comments

(a, b) -> a.id == null && b.id == null ? 0 : a.id == null ? 1 : -1 do you still believe it's a clear way? :)
That comparator breaks the interface’s contract. It may well work in practice (I haven’t tested) but it’s much safer to use the methods e.g Comparator.nullsLast() to get this right.
@cppbeginner it’s easy to fix: (a, b) -> (a.id==null) == (b.id==null)? 0: a.id==null? 1: -1, but I would use a factory method too. In this specific case, .sorted(Comparator.comparing(p -> p.id == null)) is simpler than dealing with .nullsLast() which you would have to combine with another property comparator (.sorted(Comparator.comparing(p -> p.id, Comparator.nullsLast(null))))…
By the way, the question is about searching for an ID and only an ID. There is no name search involved.
@Holger - Thanks for the assist - first time with nullsLast, never seen it before.
|
2

Since you didn’t define a behavior for the error case, you could use

Person getPersonFunction(List<Person> persons, String id) {
   return persons.stream()
        .max(Comparator.comparingInt(p -> id.equals(p.id())? 1: p.id()==null? 0: -1))
        .get(); // throws NoSuchElementException if list is empty
}

This relies on your statement that there will always be a person with a null id. Otherwise, when the list is empty, a NoSuchElementException will be thrown, but even worse, if neither, a matching id nor a null id exists, this will return an arbitrary person (the first one).

Note that a loop will be simpler and more efficient:

Person getPersonFunction(List<Person> persons, String id) {
    Person nullId = null;
    for(Person p: persons)
        if(id.equals(p.id())) return p; else if(p.id()==null) nullId = p;
    return Objects.requireNonNull(nullId, "no person with null id in list");
}

This is more efficient as it will return immediately when a match is found, while still being able to provide the fallback without another search operation.

Improve this answer

Comments

0

You can stream it again if no Person is found (which is clearly not optimal)

persons.stream()
       .filter(person -> person.getId().equals(id))
       .findFirst()
       .orElse(persons.stream()
                      .filter(person -> person.getId() == null)
                      .findFirst()
                      .orElseThrow(() -> new RuntimeException("No default person found")));

Another option is to use a stateful map to store (and remember) the Person will null id. (again doesn't sound good)

Improve this answer

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.