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

Task jv lottery #1738

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

DenDy1227
Copy link

Add lottery homework
Example of program output:
image

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 project! 😊 While your code is mostly well-implemented and meets many of the task requirements, there are a few critical issues that need to be addressed before we can approve the pull request:

  1. Ball Class: The toString() method override is missing. This is a core requirement for providing a string representation of the ball, which is essential for the task.

  2. Magic Numbers: The use of magic numbers, such as 100 in the Lottery class and 3 in the Application.java file, should be replaced with constants. This change is necessary to improve code readability and maintainability, aligning with best practices.

  3. Access Modifiers: Ensure that the constructor in the Ball class has an appropriate access modifier. This is important for encapsulation and clarity in your code.

Once these issues are addressed, your code will be in great shape! Keep up the good work, and I'm looking forward to seeing the improvements. 🚀

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

src/main/java/core/basesyntax/Application.java Outdated Show resolved Hide resolved
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 your submission! 🎉 While your code is mostly well-implemented, there are a few critical areas that need attention:

  1. Ball Class toString() Method: The toString() method currently returns null. It should provide a meaningful string representation of the Ball object, typically including the color and number of the ball.

  2. Magic Numbers:

    • In the Lottery class, the number 100 is still used directly as a magic number. It should be replaced with a constant to improve code readability and maintainability.
    • In the Application.java file, the number 3 has been correctly replaced with a constant NUMBER_OF_BALLS, which is a good practice.
  3. Access Modifiers: The constructor in the Ball class does not have an explicit access modifier. It is important to specify access modifiers for constructors to ensure proper encapsulation. Consider using public to allow instantiation from other classes, unless there is a specific reason to restrict access.

Despite these issues, I'm approving your pull request to help you move forward with your learning. Please review the comments and address these critical points in your future work to avoid similar problems. Keep up the good work and continue improving your code! 🚀

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

src/main/java/core/basesyntax/classes/Ball.java Outdated Show resolved Hide resolved
src/main/java/core/basesyntax/classes/Ball.java Outdated Show resolved Hide resolved
src/main/java/core/basesyntax/classes/Lottery.java Outdated Show resolved Hide resolved
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.

2 participants