Skip to content
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

Methods with lambda are implemented #1209

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Conversation

SSXcorp
Copy link

@SSXcorp SSXcorp commented Dec 25, 2024

No description provided.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job on implementing the task requirements effectively! 🎉 Your use of Java Streams and Predicate implementation is spot on. While there are a few areas for improvement, such as optimizing the getOddNumsAverage method with IntStream and ensuring exception messages match the task format, these are minor and don't impact the core functionality. Also, consider adding validation for the years list to enhance robustness. Keep up the excellent work and continue refining your skills! 🚀

✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨

Comment on lines 17 to 19
List<Integer> years = Arrays.stream(candidate.getPeriodsInUkr().split("-"))
.map(Integer::parseInt)
.collect(Collectors.toList());

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding validation to ensure that the years list contains exactly two elements before accessing them. This will prevent potential IndexOutOfBoundsException if the getPeriodsInUkr method does not return a valid range.

Comment on lines +22 to +23
.orElseThrow(() -> new RuntimeException("Can't get min value from list: "
+ numbers));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure that the exception message format matches the task requirement: "Can't get min value from list: " + numbers.

Comment on lines 32 to 35
.filter(n -> n % 2 != 0)
.mapToDouble(Double::valueOf)
.average()
.orElseThrow(() -> new NoSuchElementException("No odd numbers found"));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using IntStream directly to filter odd numbers, which can simplify the logic and improve performance. This aligns with the checklist advice to use a single map operation where possible.

Copy link

@okuzan okuzan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job!

return candidate.getAge() >= MINIMAL_AGE
&& candidate.isAllowedToVote()
&& candidate.getNationality().equals(NATIONALITY)
&& (years.get(1) - years.get(0)) >= MINIMAL_YEARS_IN_UA;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can make indexes constants too

.filter(VALIDATOR)
.map(Candidate::getName)
.sorted()
.collect(Collectors.toList());
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
.collect(Collectors.toList());
.toList();

it's very common, that's why the introduced .toList(), fix everywhere

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. The answer why I made code like this : On the Mate academy platform task it was impossible to use just toList() construction.

Comment on lines 51 to 55
if (person.getSex() == Person.Sex.MAN) {
return person.getAge() <= maleToAge;
} else {
return person.getAge() <= femaleToAge;
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use ternary operator to streamline this

- Constants in CandidateValidator class
- Simplified collect to List method
- Used ternary operator in getWorkablePeople method against if-else block
- Checkstyle fixes
- simplified findMinEvenNumber stream
@SSXcorp SSXcorp requested a review from okuzan December 26, 2024 15:11
Copy link

@sarakhmen sarakhmen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's slightly improve your solution :)

src/main/java/practice/CandidateValidator.java Outdated Show resolved Hide resolved
src/main/java/practice/StreamPractice.java Outdated Show resolved Hide resolved
src/main/java/practice/StreamPractice.java Outdated Show resolved Hide resolved
- Coma and dash constants represented
- changed getOddNumsAverage method impl
@SSXcorp SSXcorp requested a review from sarakhmen December 27, 2024 08:05
Comment on lines 28 to 34
.mapToDouble(i -> {
int value = numbers.get(i);
return i % 2 != 0 ? value - 1 : value;
})
.filter(n -> n % 2 != 0.0)
.average()
.getAsDouble();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
.mapToDouble(i -> {
int value = numbers.get(i);
return i % 2 != 0 ? value - 1 : value;
})
.filter(n -> n % 2 != 0.0)
.average()
.getAsDouble();
.map(i -> i % 2 == 1 ? numbers.get(i) - 1 : numbers.get(i))
.filter(n -> n % 2 == 1)
.average()
.orElseThrow();

@SSXcorp SSXcorp requested a review from Sanderfuck December 27, 2024 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants