-
Notifications
You must be signed in to change notification settings - Fork 103
Lesson 57 (stream collect collector) #103
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: for-pr
Are you sure you want to change the base?
Lesson 57 (stream collect collector) #103
Conversation
| public List<Employee> calculate(List<Department> departments) { | ||
| return null; | ||
| return departments.stream() | ||
| .flatMap(d -> d.getEmployees() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Вложенные цепочки вызовов обычно усложняют читаемость кода. Вполне можно заменить на map+flatMap
| return departments.stream() | ||
| .flatMap(d -> d.getEmployees() | ||
| .stream()) | ||
| .distinct() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
зачем?
| return null; | ||
| return departments.stream() | ||
| .flatMap(d -> d.getEmployees() | ||
| .stream()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
то же, что и выше, дальше подобное не буду дублировать
| return departments.stream() | ||
| .flatMap(d -> d.getEmployees() | ||
| .stream()) | ||
| .distinct() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
То же, что и выше
| return departments.stream() | ||
| .collect(Collectors.groupingBy(Department::getName, Collectors.flatMapping( | ||
| d -> d.getEmployees() | ||
| .stream(), Collectors.averagingDouble(Employee::getAge)))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
я очень большой сторонник правила точка-строчка, но когда стрим или другая цепочка - не единственный параметр метода, смотрится такая конструкция мутно
| .collect(Collectors.groupingBy(Department::getName, Collectors.teeing(Collectors.flatMapping( | ||
| d -> d.getEmployees() | ||
| .stream() | ||
| .filter(e -> !e.isMale()), Collectors.counting()), Collectors.flatMapping( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Collectors.flatMapping( логичнее бы на след строку перенести. Как и предыдущий
| d -> d.getEmployees() | ||
| .stream(), | ||
| Collectors.collectingAndThen(Collectors.summarizingInt(Employee::getAge), | ||
| s -> s.getMax() - s.getMin())))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
считаем лишние параметры, в остальном - элегантное решение
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Пожалуй, лучшее, чем предложено в разборе
| return departments.stream() | ||
| .flatMap(d -> d.getEmployees() | ||
| .stream()) | ||
| .filter(e -> !e.isMale()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Как насчет Predicate.not()?
| .flatMap(d -> d.getEmployees() | ||
| .stream()) | ||
| .filter(e -> !e.isMale()) | ||
| .distinct() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
зачем?)
| .stream()) | ||
| .filter(e -> !e.isMale()) | ||
| .distinct() | ||
| .collect(Collectors.collectingAndThen(Collectors.toList(), women -> women.size() > 30 ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Я в таких случаях обычно предпочитаю Optional и цепочку уже в нем отстраивать. Чуть легче разбирать
| .flatMap(d -> d.getEmployees() | ||
| .stream()) | ||
| .distinct() | ||
| .collect(Collectors.teeing(Collectors.filtering(Employee::isMale, Collectors.toList()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
teeing - штука, которую в дальнейшем советую не юзать без крайней необходимости. Бывает слишком сложно разобрать, что к чему, если логика нетривиальная. Что до лишнего списка - он, считай, идет по цене вызова фильтров и создания самого объекта списка. Т.е. там не будет именно х2 прогона каждого элемента, если тебя это беспокоило. Просто к каждому элементу при обработке применится по два набора операций
| .flatMap(d -> d.getEmployees() | ||
| .stream()) | ||
| .distinct() | ||
| .collect(Collectors.teeing(Collectors.filtering(Employee::isMale, Collectors.counting()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
кажется, перенеси ты Collectors.filtering(Employee::isMale, Collectors.counting()), - читать было бы чутка легче
| d -> d.getEmployees() | ||
| .stream() | ||
| .filter(e -> !e.isMale()) | ||
| .distinct(), Collectors.counting()), (mans, women) -> mans > women))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Можно лаконичнее
| return null; | ||
| return departments.stream() | ||
| .collect(Collectors.toMap(Department::getName, | ||
| d -> (double) d.getVacancyAmount() * 100 / d.getEmployees() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
домножение на 100 - удел отвечающих за отображение данных:) шутка, но в каждой шутке...
| return departments.stream() | ||
| .collect(Collectors.toMap(Department::getName, | ||
| d -> (double) d.getVacancyAmount() * 100 / d.getEmployees() | ||
| .size())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
лишний перенос. Цель понятна, но читаемость портит
| @Override | ||
| public Integer calculate(List<Department> departments) { | ||
| return null; | ||
| return departments.stream().mapToInt(Department::getVacancyAmount).sum(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
точка-строчка:)
| return null; | ||
| return departments.stream() | ||
| .map(Department::getName) | ||
| .collect(Collectors.joining(", ", "", ".")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Про точку речи не было)
No description provided.