Added balls, random color, random number. by Papugo-FS · Pull Request #1998 · mate-academy/jv-lottery

Papugo-FS · 2026-01-28T21:16:41Z

No description provided.

mateacademy-ai-mentor

Review Summary

Great job setting up the project structure! However, some changes are needed before this can be approved.

The most critical issue is in your Lottery class. It reuses a single Ball object for every call to getRandomBall(). This is a major logical error because the task requires creating a new, unique ball each time. A new Ball must be instantiated inside the getRandomBall() method.

Additionally, there are a few other points to address:

The random number generation is incorrect. random.nextInt(100) produces numbers up to 99, but the requirement is a maximum of 100.
In Application.java, you should use a for loop to create the three balls instead of repeating code. [CHECKLIST ITEM #8]
New instances of Random and ColorSupplier are created on every method call. These should be created once as private fields and reused. [CHECKLIST ITEM #4]

Please fix these issues, and I'll be happy to take another look!

✨ 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 · 2026-01-28T21:18:42Z

src/main/java/core/basesyntax/Application.java

+        Ball ball1 = new Lottery().getRandomBall();
+        Ball ball2 = new Lottery().getRandomBall();
+        Ball ball3 = new Lottery().getRandomBall();


This code violates two checklist items:

Checklist item Wrote an assignment jv-lottery. #4: 'Don't create several instances of the same class if you can use only one instance for your purposes'. You should create a single Lottery object before generating the balls and reuse it for each call to getRandomBall().

Checklist item Hw jv oop lotery #8: 'Use for loop for creating several objects of the same class'. Instead of creating each ball on a separate line, you should use a for loop to generate the three balls. This avoids code duplication.

mateacademy-ai-mentor · 2026-01-28T21:18:42Z

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

+                "color='" + getColor() + '\'' +
+                "number='" + getNumber() + '\'' +


The output string is missing a separator between the color and number fields, which can make it difficult to read. It's a good practice to separate fields with a comma and a space for better clarity (e.g., "color='" + color + "', number='" + number + "'").

mateacademy-ai-mentor · 2026-01-28T21:18:42Z

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

    public String getRandomColor() {
-        return null;
+
+        int randomInt = new Random().nextInt(Colors.values().length);


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 instance is created with every method call. It is more efficient to declare Random as a private field of the class and initialize it just once.

mateacademy-ai-mentor · 2026-01-28T21:18:42Z

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

+        int randomInt = new Random().nextInt(Colors.values().length);
+        Colors color = Colors.values()[randomInt];
+        return color.toString();


This block of code violates two checklist items:

Checklist item Create random colorand number of ball #2: 'Don't create temporary variables if you can directly return value from the method'. The randomInt and color variables are unnecessary.

Checklist item created simple lottery application #10: 'Use name() for getting String representation of enum constants'. You should use the name() method instead of toString().

The entire method body can be simplified into a single return statement.

mateacademy-ai-mentor · 2026-01-28T21:18:42Z

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

+import java.util.Random;
+
+public class Lottery {
+     Ball ball = new Ball();


Creating the Ball object as a class field is incorrect. This causes every call to getRandomBall() on the same Lottery instance to return the exact same object, just with updated values. The task requires creating a new ball for each call. This field should be removed, and a new Ball should be created inside the getRandomBall() method. Also, this field has a default access modifier, which violates checklist item #7: 'Classes, fields, constructors, and methods must have access modifiers'.

mateacademy-ai-mentor · 2026-01-28T21:18:42Z

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

+
+public class Lottery {
+     Ball ball = new Ball();
+     private static final int FIGURE_COUNT = 100;


The requirement states that the maximum possible number for a ball should be 100. random.nextInt(100) generates a random integer from 0 (inclusive) up to 100 (exclusive), meaning the maximum number will be 99. To include 100 in the possible range, the bound should be 101.

mateacademy-ai-mentor · 2026-01-28T21:18:42Z

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

+
+    public Ball getRandomBall() {
+
+        String randomColor = new ColorSupplier().getRandomColor();


This line has two issues:

It violates checklist item Wrote an assignment jv-lottery. #4: 'Don't create several instances of the same class if you can use only one instance for your purposes'. A new ColorSupplier instance is created every time this method is called. It would be more efficient to create a single ColorSupplier instance as a private field in the Lottery class and reuse it.

It violates checklist item Create random colorand number of ball #2: 'Don't create temporary variables if you can directly return value from the method'. The randomColor variable is unnecessary. You can set the color directly on the ball object.

mateacademy-ai-mentor

Review Summary

Great job on fixing the random number generation and using a for loop! However, some changes are needed before this can be approved.

The most critical issue is in the Lottery class. Currently, you are modifying and returning the same Ball object on every call to getRandomBall(). This is a major logical error because the goal is to create three new, unique balls. To fix this, you should create a new Ball instance (new Ball(...)) inside the getRandomBall() method and remove the ball instance field from the Lottery class.

Additionally, please address these points from the checklist:

Create the Lottery instance once before the loop in Application.java. [CHECKLIST ITEM #4]
Make ColorSupplier a class-level field in Lottery. [CHECKLIST ITEM #5]
Use .name() instead of .toString() for the enum in ColorSupplier.java. [CHECKLIST ITEM #10]
Replace the magic number 3 with a named constant in Application.java. [CHECKLIST ITEM #9]

Keep up the great work, and I look forward to seeing your updated solution!

✨ 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 · 2026-01-28T22:10:16Z

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

+
    public String getRandomColor() {
-        return null;
+        return Colors.values()[random.nextInt(Colors.values().length)].toString();


This violates checklist item #10: "Use name() for getting String representation of enum constants". The name() method is preferred for enums as it guarantees returning the constant's name, whereas toString() can be overridden.

mateacademy-ai-mentor · 2026-01-28T22:10:16Z

src/main/java/core/basesyntax/Application.java

 public class Application {
    public static void main(String[] args) {
-        // create three balls using class Lottery and print information about them in console
+        for (int i = 0; i < 3; i++) {


This violates checklist item #9: 'All magic numbers in your code should be constants.' The number 3 should be declared as a named constant to improve readability and make the code easier to maintain.

mateacademy-ai-mentor · 2026-01-28T22:10:16Z

src/main/java/core/basesyntax/Application.java

    public static void main(String[] args) {
-        // create three balls using class Lottery and print information about them in console
+        for (int i = 0; i < 3; i++) {
+            System.out.println(new Lottery().getRandomBall());


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 Lottery object should be created once, before the loop. Then, you can call its getRandomBall() method multiple times on the same instance inside the loop.

mateacademy-ai-mentor · 2026-01-28T22:10:16Z

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

+public class Lottery {
+    private static final int FIGURE_COUNT = 101;
+    private final Random random = new Random();
+    private Ball ball = new Ball();


The critical issue from the previous review has not been resolved. The Lottery class should not have a single ball instance field. Every call to getRandomBall() should create and return a completely new Ball object. Currently, you are modifying and returning the exact same object every time.

mateacademy-ai-mentor · 2026-01-28T22:10:16Z

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

+    private Ball ball = new Ball();
+
+    public Ball getRandomBall() {
+        String randomColor = new ColorSupplier().getRandomColor();


This violates checklist item #5: 'Dependencies like services should be class-level fields rather than local variables in a method'. The ColorSupplier should be instantiated once as a private field in the Lottery class and reused, just as you've done with the Random object.

mateacademy-ai-mentor

Review Summary

Great job fixing the core logic in the Lottery class! The program now works as intended, which is a big step forward. I am approving this solution as an educational override so you can move on, but please take a moment to review a few important points for future tasks.

To improve your code quality, remember to create instances only once if they can be reused. For example, the Lottery object should be created before the loop in Application.java [CHECKLIST ITEM #4], and the ColorSupplier should be a field within the Lottery class [CHECKLIST ITEM #5]. Also, avoid using "magic numbers" like 3; it's better to declare them as constants [CHECKLIST ITEM #9]. Finally, please use .name() instead of .toString() for enums, as it's a safer practice [CHECKLIST ITEM #10].

Keep up the great work, and apply these tips to your next project!

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

Rate AI review example ↩

mateacademy-ai-mentor · 2026-01-28T22:14:12Z

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

+
    public String getRandomColor() {
-        return null;
+        return Colors.values()[random.nextInt(Colors.values().length)].toString();


This violates checklist item #10: 'Use name() for getting String representation of enum constants'. As mentioned in the previous review, it's better to use the name() method for enums, as toString() can be overridden and might not return the constant's name.

mateacademy-ai-mentor · 2026-01-28T22:14:12Z

src/main/java/core/basesyntax/Application.java

 public class Application {
    public static void main(String[] args) {
-        // create three balls using class Lottery and print information about them in console
+        for (int i = 0; i < 3; i++) {


This violates checklist item #9: 'All magic numbers in your code should be constants.' The number 3 should be extracted into a private static final constant to improve readability and maintainability.

mateacademy-ai-mentor · 2026-01-28T22:14:12Z

src/main/java/core/basesyntax/Application.java

    public static void main(String[] args) {
-        // create three balls using class Lottery and print information about them in console
+        for (int i = 0; i < 3; i++) {
+            System.out.println(new Lottery().getRandomBall());


This violates checklist item #4: 'Don't create several instances of the same class if you can use only one instance for your purposes'. You should create the Lottery instance once, before the for loop begins, and then call the getRandomBall() method on that same instance within the loop.

mateacademy-ai-mentor · 2026-01-28T22:14:12Z

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

+
+    public Ball getRandomBall() {
+        Ball ball = new Ball();
+        String randomColor = new ColorSupplier().getRandomColor();


This violates checklist item #5: 'Think about which variables should be local in the method and which should be class-level'. The ColorSupplier is a dependency of the Lottery class. It should be created once as a private final field of the Lottery class, rather than creating a new instance every time getRandomBall() is called.

Added balls, random color, random number.

d8729ad

mateacademy-ai-mentor suggested changes Jan 28, 2026

View reviewed changes

small fix

58dc597

Papugo-FS requested a review from mateacademy-ai-mentor January 28, 2026 22:08

mateacademy-ai-mentor suggested changes Jan 28, 2026

View reviewed changes

small fix

871e5e6

Papugo-FS requested a review from mateacademy-ai-mentor January 28, 2026 22:12

mateacademy-ai-mentor approved these changes Jan 28, 2026

View reviewed changes

		"color='" + getColor() + '\'' +
		"number='" + getNumber() + '\'' +


		public Ball getRandomBall() {

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

Conversation

Papugo-FS commented Jan 28, 2026

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!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants