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

first #1207

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

first #1207

wants to merge 8 commits into from

Conversation

YarikNarozhnyi01
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.


Thank you for your hard work on this pull request! 🚀 While your code is mostly on the right track, there are a few critical issues that need to be addressed before we can proceed:

  1. Exception Handling: There are multiple instances where Integer.parseInt is used without proper error handling. This can lead to NumberFormatException if the input strings are not valid integers. It's important to add error handling to ensure the robustness of your code.

  2. Null Safety: Ensure that methods like getCats() return non-null lists to avoid potential NullPointerException. Consider using Optional or adding null checks where necessary.

  3. Unused Variables: The variable sum is declared but not used in the method. Removing unused variables can help clean up the code and improve readability.

  4. String Format Validation: Ensure that candidate.getPeriodsInUkr() returns a string in the expected format. If the format is not guaranteed, additional validation or error handling might be necessary to avoid runtime exceptions.

Addressing these issues will significantly improve the functionality and reliability of your code. Once these changes are made, we can move forward with the approval process. Keep up the great work, and don't hesitate to reach out if you have any questions! 😊

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

🔄 Fix the issues, commit the changes, and then re-request my review using the Re-request review button in the Reviewers tab.

Comment on lines 10 to 11
String[] splitted = candidate.getPeriodsInUkr().split("-");
int inted = Integer.parseInt(splitted[1]) - Integer.parseInt(splitted[0]);

Choose a reason for hiding this comment

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

Consider adding error handling for Integer.parseInt to prevent potential NumberFormatException if the split method does not return valid integers. This can happen if the getPeriodsInUkr method returns a malformed string.

Comment on lines 12 to 13
if (candidate.isAllowedToVote() && candidate.getAge() >= 35 && candidate.getNationality().equals("Ukrainian")
&& inted >= 10) {

Choose a reason for hiding this comment

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

Ensure that candidate.getPeriodsInUkr() returns a string in the expected format. If the format is not guaranteed, additional validation or error handling might be necessary to avoid runtime exceptions.

return numbers.stream()
.map(s -> s.split(","))
.flatMap(strings -> Arrays.stream(strings))
.map(s -> Integer.parseInt(s))

Choose a reason for hiding this comment

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

Consider adding error handling for Integer.parseInt to prevent potential NumberFormatException if the input strings are not valid integers.

@@ -23,7 +36,12 @@
* But before that subtract 1 from each element on an odd position (having the odd index).
*/
public Double getOddNumsAverage(List<Integer> numbers) {
return 0D;
Double sum = (double) 0;

Choose a reason for hiding this comment

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

The variable sum is declared but not used in the method. It can be removed to clean up the code.

@@ -59,7 +82,12 @@
* return the names of all cats whose owners are women from `femaleAge` years old inclusively.
*/
public List<String> getCatsNames(List<Person> peopleList, int femaleAge) {
return Collections.emptyList();
return peopleList.stream()
.filter(s -> s.getAge() >= femaleAge && !s.getCats().isEmpty() && s.getSex() == Person.Sex.WOMAN)

Choose a reason for hiding this comment

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

Ensure that getCats() returns a non-null list to avoid potential NullPointerException. Consider using Optional or adding a null check if necessary.

Yaroslav Narozhnyi added 2 commits December 22, 2024 00:13
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 CandidateValidator and StreamPractice classes! 🎉 While the code is generally well-structured, there are a few critical issues that need attention to enhance its robustness and reliability:

  1. Exception Handling:

    • In the findMinEvenNumber method, ensure the error message in the RuntimeException includes the actual input list numbers for better context.
    • Consider adding error handling for Integer.parseInt to prevent NumberFormatException if the input string is not a valid integer. A try-catch block can be helpful here.
  2. Code Cleanliness:

    • The variable sum in the getOddNumsAverage method is declared but not used. Removing it will clean up the code.
  3. String Format Validation:

    • Ensure that candidate.getPeriodsInUkr() returns a string in the expected format (e.g., 'YYYY-YYYY'). If the format is not guaranteed, consider adding validation or error handling to avoid runtime exceptions.
  4. Use of Constants:

    • In CandidateValidator, replace magic numbers and strings with constants for better readability and maintainability.
  5. Null Safety:

    • Ensure the getCats() method returns a non-null list to avoid potential NullPointerException. Consider adding null checks or using Optional.

These changes are necessary to ensure the code meets the specified requirements and constraints. The decision to approve is made to help you move forward with your learning. Please review the comments and address these issues to improve your code. Keep up the good work and continue learning from these experiences! 🚀

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

public class CandidateValidator implements Predicate<Candidate> {
@Override
public boolean test(Candidate candidate) {
String[] splitted = candidate.getPeriodsInUkr().split("-");

Choose a reason for hiding this comment

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

Consider adding error handling for Integer.parseInt to prevent NumberFormatException if the input string is not a valid integer. You can use a try-catch block to handle this exception.

public class CandidateValidator implements Predicate<Candidate> {
@Override
public boolean test(Candidate candidate) {
String[] splitted = candidate.getPeriodsInUkr().split("-");

Choose a reason for hiding this comment

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

Ensure that candidate.getPeriodsInUkr() returns a string in the expected format (e.g., 'YYYY-YYYY'). If the format is not guaranteed, consider adding validation or error handling to avoid runtime exceptions.

return numbers.stream()
.map(s -> s.split(","))
.flatMap(strings -> Arrays.stream(strings))
.map(s -> Integer.parseInt(s))

Choose a reason for hiding this comment

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

Consider adding error handling for Integer.parseInt to prevent NumberFormatException if the input string is not a valid integer. You can use a try-catch block to handle this exception.

Comment on lines +27 to +28
new RuntimeException("Can't get min value from list: "
+ "< Here is our input 'numbers' >"));

Choose a reason for hiding this comment

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

The error message in the RuntimeException should include the actual input list numbers to provide more context when the exception is thrown.

@@ -23,7 +34,12 @@
* But before that subtract 1 from each element on an odd position (having the odd index).
*/
public Double getOddNumsAverage(List<Integer> numbers) {
return 0D;
Double sum = (double) 0;

Choose a reason for hiding this comment

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

The variable sum is declared but not used in the getOddNumsAverage method. Consider removing it to clean up the code.

Comment on lines 89 to 92
.map(s -> s.getCats())
.flatMap(s -> s.stream())
.map(s -> s.getName())
.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.

not a mistake,but use method references where possible, use .toList()

Comment on lines 11 to 14
if (candidate.isAllowedToVote() && candidate.getAge() >= 35
&& candidate.getNationality().equals("Ukrainian")
&& inted >= 10) {
return true;
Copy link

Choose a reason for hiding this comment

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

extract to constants

@Override
public boolean test(Candidate candidate) {
String[] splitted = candidate.getPeriodsInUkr().split("-");
int inted = Integer.parseInt(splitted[1]) - Integer.parseInt(splitted[0]);
Copy link

Choose a reason for hiding this comment

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

indexes to constants too

.filter(new CandidateValidator())
.map(s -> s.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.

.toList()

Yaroslav Narozhnyi added 2 commits December 22, 2024 19:35
import model.Candidate;

public class CandidateValidator implements Predicate<Candidate> {
private static final String nationality = "Ukrainian";

Choose a reason for hiding this comment

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

let's check naming convention for constants

@Override
public boolean test(Candidate candidate) {
String[] splitted = candidate.getPeriodsInUkr().split("-");
int inted = Integer.parseInt(splitted[partTwo]) - Integer.parseInt(splitted[partOne]);

Choose a reason for hiding this comment

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

what is it inted?

return 0;
return numbers.stream()
.map(s -> s.split(","))
.flatMap(strings -> Arrays.stream(strings))

Choose a reason for hiding this comment

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

let's combine map and flatMap

@@ -35,7 +50,10 @@ public Double getOddNumsAverage(List<Integer> numbers) {
* Example: select men who can be recruited to army (from 18 to 27 years old inclusively).
*/
public List<Person> selectMenByAge(List<Person> peopleList, int fromAge, int toAge) {
return Collections.emptyList();
return peopleList.stream()
.filter(s -> s.getAge() >= fromAge && s.getAge() <= toAge

Choose a reason for hiding this comment

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

rename s

check other methods

.filter(s -> s.getAge() >= femaleAge && !s.getCats().isEmpty()
&& s.getSex() == Person.Sex.WOMAN)
.map(s -> s.getCats())
.flatMap(s -> s.stream())

Choose a reason for hiding this comment

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

combine map and flatMap

Comment on lines 20 to 21
.map(s -> s.split(","))
.flatMap(strings -> Arrays.stream(strings))

Choose a reason for hiding this comment

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

Suggested change
.map(s -> s.split(","))
.flatMap(strings -> Arrays.stream(strings))
.flatMap(input -> Arrays.stream(input.split(",")))

Comment on lines 88 to 89
.map(cat -> cat.getCats())
.flatMap(s -> s.stream())

Choose a reason for hiding this comment

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

Suggested change
.map(cat -> cat.getCats())
.flatMap(s -> s.stream())
.flatMap(person -> person.getCats().stream())

return Collections.emptyList();
return candidates.stream()
.filter(new CandidateValidator())
.map(s -> s.getName())

Choose a reason for hiding this comment

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

rename s

.flatMap(strings -> Arrays.stream(strings))
.map(s -> Integer.parseInt(s))
.filter(s -> s % 2 == 0)
.min((s,b) -> s - b)

Choose a reason for hiding this comment

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

Suggested change
.min((s,b) -> s - b)
.min(Integer::compareTo)

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! A few minor comments ;)

@@ -23,7 +33,12 @@ public int findMinEvenNumber(List<String> numbers) {
* But before that subtract 1 from each element on an odd position (having the odd index).
*/
public Double getOddNumsAverage(List<Integer> numbers) {
return 0D;
Double sum = (double) 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

remove unused variable


@Override
public boolean test(Candidate candidate) {
String[] splitted = candidate.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.

the separator also should be constant

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