Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because one core functional requirement is not met: in Application.main you create and print only one Ball, but the task explicitly requires creating and printing three balls using Lottery.getRandomBall(), and this will cause tests to fail. Please introduce a constant like private static final int NUMBER_OF_BALLS = 3; in Application and use a for loop to call lottery.getRandomBall() three times and print each result.
There are also several checklist-related issues you should fix:
- In
Lottery.getRandomBall(), replace the magic number100inrandom.nextInt(100) + 1with a named constant, e.g.private static final int MAX_BALL_NUMBER = 100;, and consider returningnew Ball(...)directly instead of using temporary variables. - In
ColorSupplier, moveRandomto a private final field (so you don’t create a new instance on each call) and simplifygetRandomColor()to avoid unnecessary temporary variables. - In
Ball.toString(), usecolor.name()when building the string to follow the enum string-representation guideline.
Functionally you are very close, and your classes (Ball, Color, ColorSupplier, Lottery) generally follow the requirements; once you fix the main logic and the checklist issues above, your solution should be ready to pass.
✨ 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
| return "Ball{color=" | ||
| + color | ||
| + ", number=" | ||
| + number |
There was a problem hiding this comment.
This toString() uses the enum value via concatenation (which calls toString() implicitly). The checklist requires: "Use name() for getting String representation of enum constants. 'Don't use toString() or String.valueOf() for getting String representation of enum constants.'" Consider using color.name() when building the string to follow the guideline.
| public static void main(String[] args) { | ||
| // create three balls using class Lottery and print information about them in console | ||
| Lottery lottery = new Lottery(); | ||
| System.out.println(lottery.getRandomBall()); |
There was a problem hiding this comment.
This prints only a single Ball, but the task requires: "In the main() method, create three balls using the getRandomBall() method from the Lottery class. And print information about them in the console." Also this violates checklist item #8: "Use for loop for creating several objects of the same class" — use a constant (e.g. private static final int NUMBER_OF_BALLS = 3) and a for loop to call lottery.getRandomBall() three times and print each result. Refer to the task description for the exact requirement.
| return null; | ||
| public Color getRandomColor() { | ||
| Color[] colors = Color.values(); | ||
| Random random = new Random(); |
There was a problem hiding this comment.
This creates a new Random instance on each call. It violates checklist item #4: "Don't create several instances of the same class if you can use only one instance for your purposes" and checklist item #5: "Think about which variables should be local in the method and which should be class-level". Move Random to a private final field of the class and reuse it.
| public String getRandomColor() { | ||
| return null; | ||
| public Color getRandomColor() { | ||
| Color[] colors = Color.values(); |
There was a problem hiding this comment.
The method creates temporary variables (colors, index) that can be avoided. This violates checklist item #2: "Don't create temporary variables if you can directly return value from the method". Consider simplifying the method to return directly (for example, using the class-level Random) or otherwise reduce temporaries.
| Color randomColor = colorSupplier.getRandomColor(); | ||
| int randomNumber = random.nextInt(100) + 1; | ||
| return new Ball(randomColor, randomNumber); |
There was a problem hiding this comment.
This method creates temporary local variables (randomColor, randomNumber) and then returns a new Ball. This violates checklist item #2: "Don't create temporary variables if you can directly return value from the method." Consider returning directly, e.g.
return new Ball(colorSupplier.getRandomColor(), random.nextInt(MAX_BALL_NUMBER) + 1); which removes unnecessary temporaries and is clearer. See the Lottery implementation for context.
|
|
||
| public Ball getRandomBall() { | ||
| Color randomColor = colorSupplier.getRandomColor(); | ||
| int randomNumber = random.nextInt(100) + 1; |
There was a problem hiding this comment.
The literal 100 is used directly in random.nextInt(100) + 1. This violates checklist item #9: "All magic numbers in your code should be constants." Define a named constant such as private static final int MAX_BALL_NUMBER = 100 and use it here: random.nextInt(MAX_BALL_NUMBER) + 1. This improves readability and follows the style guide.
|
I have changed marked points, please recheck and let me know. |
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your solution because all functional and checklist requirements for this task are met: the Application class creates and prints three balls using a single Lottery instance, Lottery generates a Ball with a random color and number in the correct range, ColorSupplier correctly returns random enum values, and the Ball and Color classes are implemented according to the specification (fields color and number, toString() using color.name(), proper enum values, and use of constants instead of magic numbers). None of the identified issues break functionality or violate a critical requirement.
The remaining points are minor style improvements:
- In
Ball.java, remove redundant empty lines (after the package declaration, between fields and constructor, and between the constructor and thetoString()method) to align with [CHECKLIST ITEM #1]. - In
ColorSupplier.java, you can simplifygetRandomColor()by returning the value directly instead of storingColor.values()in a temporary variable, e.g.return Color.values()[random.nextInt(Color.values().length)];to satisfy [CHECKLIST ITEM #2].
Overall, your implementation is clean, readable, and fulfills the task requirements; just keep an eye on these small style details in future tasks.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| @@ -0,0 +1,20 @@ | |||
| package core.basesyntax; | |||
|
|
|||
| public class Ball { | ||
| private final Color color; | ||
| private final int number; | ||
|
|
| this.color = color; | ||
| this.number = number; | ||
| } | ||
|
|
| Color[] colors = Color.values(); | ||
| return colors[random.nextInt(colors.length)]; |
There was a problem hiding this comment.
There were changed a bit: Lottery / Ball / Color classes