Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job on implementing the core functionality of the lottery program! The Ball and Color classes are well-structured. However, there are several violations of the project checklist that need to be addressed before this can be approved.
Here are the required changes:
- In
Lottery.javaandColorSupplier.java, theRandomobject should be created once as a private field rather than inside a method that gets called repeatedly. [CHECKLIST ITEM #4] - The hardcoded numbers
3inApplication.javaand100inLottery.javaare magic numbers and must be declared as named constants. [CHECKLIST ITEM #9] - The code for creating balls in
Application.javais repetitive. Please use aforloop as specified in the checklist. [CHECKLIST ITEM #8] - The logic for the random number in
Lottery.javagenerates a value up to 99, but the requirement is a maximum of 100. Please adjust this.
You're very close to a perfect solution. Keep up the great work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| Ball randomBall1 = lottery.getRandomBall(); | ||
| System.out.println(randomBall1.toString()); | ||
| Ball randomBall2 = lottery.getRandomBall(); | ||
| System.out.println(randomBall2.toString()); | ||
| Ball randomBall3 = lottery.getRandomBall(); | ||
| System.out.println(randomBall3.toString()); |
There was a problem hiding this comment.
This code is repetitive. According to checklist item #8, you should use a for loop to create multiple objects. A loop would make the code cleaner and easier to modify if you needed to create a different number of balls. Also, the number of balls (3) is a magic number and should be a constant, as per checklist item #9.
| public class ColorSupplier { | ||
| public String getRandomColor() { | ||
| return null; | ||
| Random random = new Random(); |
There was a problem hiding this comment.
This violates checklist item #4: 'Don't create several instances of the same class if you can use only one instance for your purposes'. The Random object should be created once as a field of the ColorSupplier class, rather than creating a new instance on every method call.
| Color[] colors = Color.values(); | ||
| Color randomColor = colors[random.nextInt(colors.length)]; | ||
| return randomColor.name(); |
There was a problem hiding this comment.
This violates checklist item #2: 'Don't create temporary variables if you can directly return value from the method'. The logic on lines 8-10 can be combined into a single return statement, avoiding the use of the colors and randomColor temporary variables.
| } | ||
|
|
||
| public Ball getRandomBall() { | ||
| Random random = new Random(); |
There was a problem hiding this comment.
This violates checklist item #4: 'Don't create several instances of the same class if you can use only one instance for your purposes'. A new Random object is created every time this method is called. It's better practice to declare random as a private field of the Lottery class and initialize it once.
| public Ball getRandomBall() { | ||
| Random random = new Random(); | ||
| Ball ball = new Ball(); | ||
| ball.setNumber(random.nextInt(100)); |
There was a problem hiding this comment.
This violates checklist item #9: 'All magic numbers in your code should be constants.' The number 100 should be declared as a named constant to improve readability and maintainability. For example: private static final int MAX_BALL_NUMBER = 100;.
Also, note that random.nextInt(100) generates a number in the range of 0 to 99. The requirement is that "the maximum possible number should be 100". You might need to adjust the logic to make sure the number 100 is included in the possible outcomes.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Good progress on this task! You've correctly implemented the for loop and addressed some of the previous feedback. However, I'm requesting a few changes as there are still some critical issues that prevent this from being approved.
Here's what needs to be fixed:
- Incorrect Random Number Range: In
Lottery.java, the methodrandom.nextInt(100)generates numbers from 0 to 99. The task requires the maximum possible number to be 100, so you'll need to adjust this logic. - Inefficient
RandomInstantiation: InColorSupplier.java, a newRandomobject is created on each call togetRandomColor(). This violates the guidelines [CHECKLIST ITEM #4]. Please declare it as aprivatefield of the class, just as you did inLottery. - Constant Naming and Placement: The constants in
Lottery.javaandApplication.javashould be named inUPPER_SNAKE_CASE(e.g.,MAX_BALL_NUMBER). Additionally, the constant inApplication.javashould be aprivate static finalfield at the class level. [CHECKLIST ITEM #6, #9]
You're very close to finishing. Please address these points, and I'll be happy to review it again!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| public class Application { | ||
| public static void main(String[] args) { | ||
| // create three balls using class Lottery and print information about them in console | ||
| final byte numberOfIteration = 3; |
There was a problem hiding this comment.
This is a good improvement to remove the magic number. However, according to the style guide (Checklist items #6 and #9), constants should be declared as private static final fields at the class level and named in UPPER_SNAKE_CASE. Please declare it outside the main method like this:
private static final int NUMBER_OF_ITERATIONS = 3;|
|
||
| public String getRandomColor() { | ||
| return null; | ||
| Random random = new Random(); |
There was a problem hiding this comment.
This violates checklist item #4: "Don't create several instances of the same class if you can use only one instance for your purposes". The Random object should be created only once as a private field of the class, similar to how it's done in the Lottery class.
| import java.util.Random; | ||
|
|
||
| public class Lottery extends ColorSupplier { | ||
| private static final int maxBallNumber = 100; |
There was a problem hiding this comment.
This violates checklist item #6: "Don't forget how to name constants according to style guide". Constant names should be in UPPER_SNAKE_CASE. For example, MAX_BALL_NUMBER.
|
|
||
| public Ball getRandomBall() { | ||
| Ball ball = new Ball(); | ||
| ball.setNumber(random.nextInt(maxBallNumber)); |
There was a problem hiding this comment.
The random number generation logic is still not quite right. random.nextInt(100) will generate a number in the range of 0 to 99. The requirement is for the maximum possible number to be 100 (inclusive). You'll need to adjust the argument passed to nextInt() to achieve this.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Sorry, the pull request review was unsuccessful for the following reasons:
Warning
No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
[CODE: NCM]
If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on this task! You've successfully addressed all the logical issues from the previous review, and the code now correctly implements all requirements. I am approving your solution.
There is one minor code style issue causing the build to fail, but I'm making an educational exception to help you move forward. In src/main/java/core/basesyntax/Lottery.java, you just need to add a blank line before the constructor to separate it from the field declarations. This is a common formatting rule to improve code readability.
// Before
private final Random random = new Random();
public Lottery() {
// After
private final Random random = new Random();
public Lottery() {This is a very small detail, and overall you did a fantastic job. Keep up the great work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
No description provided.