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

stream practice #1211

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

stream practice #1211

wants to merge 2 commits into from

Conversation

DariaBakal
Copy link

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 solution! 🎉 Your code effectively meets the task requirements, and you've made excellent use of the Stream API and Predicate interface. While there is a minor issue with the residency period calculation in the CandidateValidator class, it doesn't significantly impact the overall functionality. Keep up the good work, and consider refining the residency logic to handle multiple periods for future improvements. Keep coding and learning! 🚀

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

Comment on lines 31 to 35
String[] splitedPeriod = c.getPeriodsInUkr().split("-");
int startYear = Integer.parseInt(splitedPeriod[0]);
int endYear = Integer.parseInt(splitedPeriod[1]);

return (endYear - startYear) >= PERIOD_IN_UKRAINE;

Choose a reason for hiding this comment

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

The current logic only checks if the candidate has lived in Ukraine for a continuous period of 10 years based on a single period string. However, the task description implies that the candidate should have lived in Ukraine for a total of 10 years, which may not be continuous. Consider modifying the logic to sum up all the years from multiple periods if applicable.

Copy link
Contributor

@liliia-ponomarenko liliia-ponomarenko 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! Let’s improve your solution ;)

Comment on lines 22 to 30
if (c.getAge() < MIN_AGE) {
return false;
}
if (!c.isAllowedToVote()) {
return false;
}
if (!NATIONALITY.equals(c.getNationality())) {
return false;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you combine these conditionals?

if (!NATIONALITY.equals(c.getNationality())) {
return false;
}
String[] splitedPeriod = c.getPeriodsInUkr().split("-");
Copy link
Contributor

Choose a reason for hiding this comment

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

make the separator a constant field

}

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

Comment on lines 39 to 45
IntStream.range(0, numbers.size())
.filter(i -> i % 2 != 0)
.forEach(i -> copiedList.set(i, copiedList.get(i) - 1));
return copiedList.stream()
.filter(num -> num % 2 != 0)
.mapToInt(num -> num)
.average()
Copy link
Contributor

Choose a reason for hiding this comment

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

Let’s think about how to simplify it and use one stream

.filter(num -> num % 2 != 0)
.mapToInt(num -> num)
.average()
.orElseThrow(NoSuchElementException::new);
Copy link
Contributor

Choose a reason for hiding this comment

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

add informative message

Comment on lines 58 to 60
.filter(Objects::nonNull)
.filter(p -> p.getSex() == Sex.MAN)
.filter(p -> p.getAge() >= fromAge && p.getAge() <= toAge)
Copy link
Contributor

Choose a reason for hiding this comment

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

can you combine conditionals?

Comment on lines 76 to 80
.filter(Objects::nonNull)
.filter(p -> p.getAge() >= fromAge)
.filter(p -> (p.getSex() == Sex.WOMAN && p.getAge() <= femaleToAge)
|| (p.getSex() == Sex.MAN && p.getAge() <= maleToAge))
.toList();
Copy link
Contributor

Choose a reason for hiding this comment

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

the same. for complicated conditional, it is better to use Predicate

*/
public List<String> validateCandidates(List<Candidate> candidates) {
return Collections.emptyList();
return candidates.stream()
.filter(new CandidateValidator())
Copy link
Contributor

Choose a reason for hiding this comment

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

Better make it a class-level variable

}

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

Copy link
Contributor

@liliia-ponomarenko liliia-ponomarenko left a comment

Choose a reason for hiding this comment

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

Great 😊


public class StreamPractice {
public static final String SEPARATOR = ",";
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public static final String SEPARATOR = ",";
private static final String SEPARATOR = ",";

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.

3 participants