Add lottery implementation #1943

neuklearna · 2025-10-21T14:04:54Z

No description provided.

mateacademy-ai-mentor

Review Summary

I am requesting changes because several checklist-enforced requirements are violated even though the app runs. Critical items to fix:

In ColorSupplier.java: avoid creating a new Random and Colors.values() array on each call (lines where new Random() and Colors.values() are used). Make private final Random random and reuse a private static final Colors[] COLORS = Colors.values(), and return directly with COLORS[random.nextInt(COLORS.length)].
In Ball.java: fields lack access modifiers (make them private) and toString() should use color.name() instead of relying on enum's default toString().
In Application.java: replace the three repeated calls with a for-loop and introduce a private static final int BALL_COUNT = 3 constant for the count.
In Lottery.java: remove package-private class fields used as temporaries (make them local to getRandomBall()), reuse a single Random and ColorSupplier as private fields, replace magic number 101 with a private static final int MAX_NUMBER = 100, and return the Ball directly without a temporary variable.
Minor improvements: ensure all classes/fields/methods have explicit access modifiers and keep constants in UPPER_SNAKE_CASE. Once these are addressed, the solution will meet the task’s requirements. Keep up the good work—your overall structure is solid; it just needs these checklist-aligned refinements.

✨ Please rate this review in the Platform's notification center¹. 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 button² 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 tag³.
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.

mateacademy-ai-mentor · 2025-10-21T14:11:24Z

src/main/java/core/basesyntax/ColorSupplier.java

-    public String getRandomColor() {
-        return null;
+    public Colors getRandomColor() {
+        Random rand = new Random();


This line creates a fresh Random instance on every method call. This violates the checklist: "Don't create several instances of the same class if you can use only one instance for your purposes". Prefer a reusable field such as private final Random random = new Random(); to avoid repeated allocations.

mateacademy-ai-mentor · 2025-10-21T14:11:24Z

src/main/java/core/basesyntax/ColorSupplier.java

-        return null;
+    public Colors getRandomColor() {
+        Random rand = new Random();
+        Colors [] colors = Colors.values();


This creates a new array from Colors.values() each invocation and stores it in a temporary variable. The checklist says: "Don't create temporary variables if you can directly return value from the method" — consider using a private static final Colors[] COLORS = Colors.values(); field and reuse it, or return directly with COLORS[random.nextInt(COLORS.length)].

mateacademy-ai-mentor · 2025-10-21T14:11:24Z

src/main/java/core/basesyntax/ColorSupplier.java

+        int index = rand.nextInt(colors.length);
+        return colors[index];


Using a separate index local and then returning colors[index] creates unnecessary temporaries. The checklist: "Don't create temporary variables if you can directly return value from the method" — construct and return the selected color in one expression (and reuse a class-level Random and Colors array). Example: return COLORS[random.nextInt(COLORS.length)];.

mateacademy-ai-mentor · 2025-10-21T14:11:24Z

src/main/java/core/basesyntax/Ball.java

+    Colors color;
+    int number;


The fields have no explicit access modifiers. This violates the checklist: "Pay attention to access modifiers Classes, fields, constructors, methods must have access modifiers otherwise default will be used that isn't a good practice." Consider making the fields private (e.g., private Colors color; private int number;).

mateacademy-ai-mentor · 2025-10-21T14:11:24Z

src/main/java/core/basesyntax/Ball.java

+
+    @Override
+    public String toString () {
+        return "Ball [color=" + color + ", number=" + number + "]";


The toString implementation uses the enum value directly which relies on toString(). The checklist requires: "Use name() for getting String representation of enum constants" — change to color.name() to ensure the concrete enum name is used (for example: "Ball [color=" + color.name() + ", number=" + number + "]").

mateacademy-ai-mentor · 2025-10-21T14:11:25Z

src/main/java/core/basesyntax/Lottery.java

+    Colors randomColor;
+    int randomNumber;


These fields are declared without explicit access modifiers and are mutable at class level. This violates the checklist: "Pay attention to access modifiers Classes, fields, constructors, methods must have access modifiers otherwise default will be used that isn't a good practice." Consider removing these as fields and making them local, or at least make them private with clear intent.

mateacademy-ai-mentor · 2025-10-21T14:11:25Z

src/main/java/core/basesyntax/Lottery.java

+        ColorSupplier colorSupplier = new ColorSupplier();
+        randomColor = colorSupplier.getRandomColor();


A new ColorSupplier is created on each call. This violates: "Don't create several instances of the same class if you can use only one instance for your purposes" and the guidance to think about which variables should be class-level vs local: "Think about which variables should be local in the method and which should be class-level." Make ColorSupplier a reusable field instead of instantiating it here.

mateacademy-ai-mentor · 2025-10-21T14:11:25Z

src/main/java/core/basesyntax/Lottery.java

+    public Ball getRandomBall (){
+        ColorSupplier colorSupplier = new ColorSupplier();
+        randomColor = colorSupplier.getRandomColor();
+        Random rand = new Random();


A new Random is created on each call. This also violates: "Don't create several instances of the same class if you can use only one instance for your purposes" and the advice to prefer class-level helpers. Move Random to a private field and reuse it.

mateacademy-ai-mentor · 2025-10-21T14:11:25Z

src/main/java/core/basesyntax/Lottery.java

+        ColorSupplier colorSupplier = new ColorSupplier();
+        randomColor = colorSupplier.getRandomColor();
+        Random rand = new Random();
+        randomNumber = rand.nextInt(101);


A magic number 101 is used here. The checklist requires: "All magic numbers in your code should be constants." Introduce private static final int MAX_NUMBER = 100 and use random.nextInt(MAX_NUMBER + 1) (or similar) to make intent explicit.

mateacademy-ai-mentor · 2025-10-21T14:11:25Z

src/main/java/core/basesyntax/Lottery.java

+        Ball ball = new Ball(randomColor,randomNumber);
+        return ball;


You create a temporary variable ball and then return it. This violates: "Don't create temporary variables if you can directly return value from the method." Construct and return the Ball directly (e.g. return new Ball(color, number);).

mateacademy-ai-mentor

Review Summary

I am requesting changes because one critical requirement is not met and it blocks task completion. In src/main/java/core/basesyntax/Lottery.java, the random number generation uses random.nextInt(MAX_NUMBER), which yields 0..99, so 100 is never produced. The requirement states the maximum possible number must be 100. Please update the line in getRandomBall() to allow 0..100 (e.g., random.nextInt(MAX_NUMBER + 1)) or 1..100 (e.g., random.nextInt(MAX_NUMBER) + 1) per your intended range.

Additionally, there are Checkstyle errors that will fail the build (not functional blockers but will break CI):

Lottery.java: remove the extra space before ( in public Ball getRandomBall() and ensure a single space before {.
Ball.java: remove extra spaces in the constructor and toString() signatures and fix double-space in the return line.
Application.java: fix indentation of the for-loop block to 4-space indentation per level.
ColorSupplier.java: add a newline at end of file.

Functionally, the rest looks solid: enums, object structure, reuse of Random/ColorSupplier, and main loop are correct. Make these fixes and you should be good to go—nice work overall!

✨ Please rate this review in the Platform's notification center¹. 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 button² 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 tag³.
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.

mateacademy-ai-mentor · 2025-10-21T15:00:28Z

src/main/java/core/basesyntax/Lottery.java

+    private final ColorSupplier colorSupplier = new ColorSupplier();
+
+    public Ball getRandomBall (){
+        return new Ball(colorSupplier.getRandomColor(), random.nextInt(MAX_NUMBER));


This violates the checklist requirement: "Maximum number constraint: the random number assigned to a ball must not exceed 100." Using random.nextInt(MAX_NUMBER) produces values in the range 0..(MAX_NUMBER-1), so 100 is never produced. Change to random.nextInt(MAX_NUMBER + 1) to allow 0..100 (or random.nextInt(MAX_NUMBER) + 1 if you prefer 1..100). See the getRandomBall() implementation in Lottery.java for the line to update.

mateacademy-ai-mentor

Review Summary

Your solution is approved. You implemented the requirements correctly: the Ball class has proper fields and toString(), Colors is a clean enum, ColorSupplier uses Colors.values() with a shared Random, and Lottery#getRandomBall() returns a Ball with a reachable 0–100 range.

Educational override: I’m approving to help you progress while learning; please address the style items below in your next iteration.

Critical: In Application.java, fix indentation of the for loop inside main (around the for line and its block, e.g., lines ~12–16) to follow 4-space indentation. This aligns with standard style checks and prevents CI failures. Hint: ensure the for and its body align under the method’s opening brace.
Minor: Remove the extra blank line between BALL_COUNT and main (around line ~9) to avoid “redundant empty line” style warnings. Also ensure each file ends with a single newline.

Overall, great job using constants, reusing Random, and keeping logic instance-based. Keep up the solid work and just tidy up the minor formatting to meet style checks.

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

Rate AI review example ↩

Add lottery implementation

e80350a

mateacademy-ai-mentor suggested changes Oct 21, 2025

View reviewed changes

Corrected code

d6f6b2a

neuklearna requested a review from mateacademy-ai-mentor October 21, 2025 14:54

mateacademy-ai-mentor suggested changes Oct 21, 2025

View reviewed changes

Second corrected code - removed extra spaces

c0717ac

neuklearna requested a review from mateacademy-ai-mentor October 21, 2025 15:09

mateacademy-ai-mentor approved these changes Oct 21, 2025

View reviewed changes

		int index = rand.nextInt(colors.length);
		return colors[index];

		ColorSupplier colorSupplier = new ColorSupplier();
		randomColor = colorSupplier.getRandomColor();

Add lottery implementation #1943

Are you sure you want to change the base?

Add lottery implementation #1943

Uh oh!

Conversation

neuklearna commented Oct 21, 2025

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants