Using Java 8 streams for finding Persons with maximum numbers (Codeforces-151B)

Question

See Problem - 151B - Codeforces for details. I think I have been very messy with using the Streams. Can you review for a good and better implementation with Streams? Any other improvements?

public void solve(InputReader in, PrintWriter out) {
 int n = in.nextInt();
 List<Person> ps = new ArrayList<>();
 for (int i = 0; i < n; i++) {
 int pn = in.nextInt();
 String nm = in.next();
 int tN = 0, pN = 0;
 for (int j = 0; j < pn; j++) {
 String sN = in .nextLine()
 .replaceAll("-", "");
 if (taxinum(sN)) {
 tN++;
 } else if (pizzanum(sN)) {
 pN++;
 }
 }
 ps.add(new Person(nm, pn - tN - pN, tN, pN));
 }
 int maxt = ps .stream()
 .max((x, y) -> x.tN - y.tN)
 .get().tN;
 int maxg = ps .stream()
 .max((x, y) -> x.gN - y.gN)
 .get().gN;
 int maxp = ps .stream()
 .max((x, y) -> x.pN - y.pN)
 .get().pN;
 String tns = ps .stream()
 .filter(x -> x.tN == maxt)
 .map(x -> x.name)
 .collect(Collectors.joining(", "));
 String pns = ps .stream()
 .filter(x -> x.pN == maxp)
 .map(x -> x.name)
 .collect(Collectors.joining(", "));
 String gns = ps .stream()
 .filter(x -> x.gN == maxg)
 .map(x -> x.name)
 .collect(Collectors.joining(", "));
 out.println("If you want to call a taxi, you should call: " + tns + ".");
 out.println("If you want to order a pizza, you should call: " + pns + ".");
 out.println("If you want to go to a cafe with a wonderful girl, you should call: " + gns + ".");
}
private boolean taxinum(String s) {
 for (int i = 1; i < s.length(); i++) {
 if (s.charAt(i) != s.charAt(i - 1)) {
 return false;
 }
 }
 return true;
}
private boolean pizzanum(String s) {
 for (int i = 1; i < s.length(); i++) {
 if (s.charAt(i) >= s.charAt(i - 1)) {
 return false;
 }
 }
 return true;
}
public class Person {
 String name;
 int gN, tN, pN;
 public Person(String n, int g, int t, int p) {
 name = n;
 gN = g;
 tN = t;
 pN = p;
 }
}

• Mario Santini: Instead of maxt, maxg and maxp, you should get the related Person objects, so you don't need to loop again for the names.
• RE60K: @MarioAlexandroSantini there can be multiple names. so I needed to first find the maximum.
• Mario Santini: My bad, I missed the collect() row.
• Vogel612: This question is incomplete. To help reviewers give you better answers, please add sufficient context to your question. The more you tell us about what your code does and what the purpose of doing that is, the easier it will be for reviewers to help you. Questions should include a description of what the code does
• Simon Forsberg: You might want to read Simon's guide to posting a good question. In your case, you can improve on "Describe the details and your approach", "Class/Method summary", and considering your variable names probably also "Don't assume that everyone knows what you are talking about".

Answer 1

As @skiwi has stated your code could use a lot of cleaning up.

Specifically regarding the streams in your code, they could be simplified.

Firstly, you could create getters in your Person class:

public final class Person {
 private final String name;
 private final int girlNumberCount, taxiNumberCount, pizzaNumberCount;
 Person(final String name, final int girlNumberCount, final int taxiNumberCount, final int pizzaNumberCount) {
 this.name = name;
 this.girlNumberCount = girlNumberCount;
 this.taxiNumberCount = taxiNumberCount;
 this.pizzaNumberCount = pizzaNumberCount;
 }
 public String getName() {
 return name;
 }
 public int getGirlNumberCount() {
 return girlNumberCount;
 }
 public int getTaxiNumberCount() {
 return taxiNumberCount;
 }
 public int getPizzaNumberCount() {
 return pizzaNumberCount;
 }
}

Having getters like this means that you can use method references, which look a bit nicer. For example .map(Person::getTaxiNumberCount) instead of .map(person -> person.taxiNumberCount)

You can concatenate your stream for finding the max taxi count, and your stream for finding all the people with that max, and also the printing of those people:

people.stream()
 .map(Person::getTaxiNumberCount)
 .max(Integer::compare)
 .map(maxTaxiCount ->
 people.stream()
 .filter(p -> p.getTaxiNumberCount() == maxTaxiCount)
 .map(Person::getName)
 .collect(Collectors.joining(", "))
 ).ifPresent(peopleStr -> out.println("If you want to call a taxi, you should call: " + peopleStr + "."));

And the same applies to finding the max pizza count etc.

You could also replace the for loop in taxiNum(String s) method with a simple Regex pattern that matches 6 of the same number:

private boolean isTaxiNumber(final String number) {
 return Pattern.compile("(\\d)\1円{6}").matcher(number).matches();
}

After refactoring:

import java.io.PrintWriter;
import java.util.ArrayList;
import java.util.List;
import java.util.regex.Pattern;
import java.util.stream.Collectors;
public final class Example {
 public final void solve(final InputReader in, final PrintWriter out) {
 outputMax(getFriends(in), out);
 }
 private List<Person> getFriends(final InputReader in) {
 final int numFriends = in.nextInt();
 final List<Person> friends = new ArrayList<>();
 for (int i = 0; i < numFriends; i++) {
 final int friendPhoneNumberCount = in.nextInt();
 final String friendName = in.next();
 int taxiNumberCount = 0, pizzaNumberCount = 0, girlNumberCount = 0;
 for (int j = 0; j < friendPhoneNumberCount; j++) {
 final String phoneNumber = in.nextLine().replaceAll("-", "");
 if (isTaxiNumber(phoneNumber)) taxiNumberCount ++;
 else if (isPizzaNumber(phoneNumber)) pizzaNumberCount ++;
 else girlNumberCount ++;
 }
 friends.add(new Person(friendName, girlNumberCount, taxiNumberCount, pizzaNumberCount));
 }
 return friends;
 }
 private boolean isTaxiNumber(final String number) {
 return Pattern.compile("(\\d)\1円{6}").matcher(number).matches();
 }
 private boolean isPizzaNumber(final String number) {
 for (int i = 1; i < number.length(); i++) {
 if (number.charAt(i) >= number.charAt(i - 1)) {
 return false;
 }
 }
 return true;
 }
 private void outputMax(final List<Person> friends, final PrintWriter out) {
 friends.stream()
 .map(Person::getTaxiNumberCount)
 .max(Integer::compare)
 .map(maxTaxiCount ->
 friends.stream()
 .filter(p -> p.getTaxiNumberCount() == maxTaxiCount)
 .map(Person::getName)
 .collect(Collectors.joining(", "))
 ).ifPresent(peopleStr -> out.println("If you want to call a taxi, you should call: " + peopleStr + "."));
 friends.stream()
 .map(Person::getPizzaNumberCount)
 .max(Integer::compare)
 .map(maxPizzaCount ->
 friends.stream()
 .filter(p -> p.getPizzaNumberCount() == maxPizzaCount)
 .map(Person::getName)
 .collect(Collectors.joining(", "))
 ).ifPresent(peopleStr -> out.println("If you want to order a pizza, you should call: " + peopleStr + "."));
 friends.stream()
 .map(Person::getGirlNumberCount)
 .max(Integer::compare)
 .map(maxGirlCount ->
 friends.stream()
 .filter(p -> p.getGirlNumberCount() == maxGirlCount)
 .map(Person::getName)
 .collect(Collectors.joining(", "))
 ).ifPresent(peopleStr -> out.println("If you want to go to a cafe with a wonderful girl, you should call: " + peopleStr + "."));
 }
}
final class Person {
 private final String name;
 private final int girlNumberCount, taxiNumberCount, pizzaNumberCount;
 Person(final String name, final int girlNumberCount, final int taxiNumberCount, final int pizzaNumberCount) {
 this.name = name;
 this.girlNumberCount = girlNumberCount;
 this.taxiNumberCount = taxiNumberCount;
 this.pizzaNumberCount = pizzaNumberCount;
 }
 final String getName() { return name; }
 final int getGirlNumberCount() { return girlNumberCount; }
 final int getTaxiNumberCount() { return taxiNumberCount; }
 final int getPizzaNumberCount() { return pizzaNumberCount; }
}

As you can see, the Stream solution here is still quite messy and could use some improvement.

Answer 2

I have spotted too many simple issues in order to review this code in-depth, therefore I will point out what simple issues I am seeing and I encourage you to post a follow-up question such that the code can be properly reviewed:

1. Your variables names are way too short, last time I have checked there is no penalty for using longer variable names. Variable names should be descriptive, I or any other future reader of your code is not going to figure out what each variable really means. Examples of these variable names are:

   • n
   • pn
   • nm
   • tN
   • pN
   • sN
   • and many more...

   I could probably take a guess at what some of them mean, but you really should be one naming them properly. I would expect List<Person> persons = new ArrayList<>(); for example, not something named pn.

2. You should not put multiple declarations on the same line, as observed in int tN = 0, pN = 0;.

3. Your method names taxinum and pizzanum are very confusing, they sound like some mathematical operations to me and looking at the context I cannot figure out what they are supposed to mean.

4. Your indenting is off at various places, there should be no white-space on the same line when calling methods, like in in .nextLine(), you should be happy the compiler even accepts this.

That is all advice I can offer you right now. Please rewrite your code and post it again as a new question such that for example your Java 8 streams usage can be reviewed.

Lastly I want to make it clear again that you cannot continue in this way, look at this line:

ps.add(new Person(nm, pn - tN - pN, tN, pN));

How is anyone reading this code for the first time really supposed to understand this?

• Simon Forsberg: Have you read the problem description?
• skiwi: @SimonForsberg Yes, but I fail to see how it is relevant to this review.
• Simon Forsberg: After reading the problem description, I could understand the variable names pretty easy. I do agree that they are not optimal though.

Answer 3

Using streams is not always the best option. Using a greedy algorithm you only have to loop through all your persons once. You can even implement it in such a way that you don't even have to save all your persons at all.

So what do we need to keep track of, then?

• Current person name
• Current person's numbers (number of taxis, girls, pizzas)
• The best person(s) so far for each category.

So how to accomplish this?

• Initialize three lists (one for each category) for the person names.
• Initialize three numbers (one for each category), set them all to zero. This is the "best amount of numbers so far" in each category.
• Read a person's name
• Read the person's numbers and count the amount of numbers in each category
• If this person has the same number of girls, taxis or pizzas as the previous record holder, add this person to the list of persons to call for this category.
• If this person sets a new record in any category, wipe the list of the best persons for that category and only add this person.

At the end, you will have three lists holding the names of the best people to call in each category.