Skip to content

OOP Lottery Practice#2020

Open
NikitaZhovnirenko wants to merge 7 commits intomate-academy:masterfrom
NikitaZhovnirenko:master
Open

OOP Lottery Practice#2020
NikitaZhovnirenko wants to merge 7 commits intomate-academy:masterfrom
NikitaZhovnirenko:master

Conversation

@NikitaZhovnirenko
Copy link

Implemented CollorSupplier, Colors enum, Lottery class with required methods.

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.

Review Summary

I am requesting changes because, while the core functionality works (three balls are created and printed, random colors and numbers are generated), several explicit checklist requirements are not yet met, and those are considered mandatory for task completion. The main blockers are:

  • Application.main directly calls Lottery.getRandomBall() three times instead of using a for loop with a constant count, which violates [CHECKLIST ITEM #8]; you should define something like private static final int NUMBER_OF_BALLS = 3; and loop to create/print each ball.
  • ColorSupplier.getRandomColor() and Lottery.getRandomBall() are static and also create new Random instances every call, which violates [CHECKLIST ITEM #3], [#4], and [#5]; make them instance methods, store Random as a private field (e.g. private final Random random = new Random();), and call them via created objects.
  • In Lottery, the magic number 101 in nextInt(101) violates [CHECKLIST ITEM #9]; introduce a constant like private static final int MAX_BALL_NUMBER = 100; and use random.nextInt(MAX_BALL_NUMBER + 1).
  • In ColorSupplier, using toString() on the enum and the temporary Colors[] colors = Colors.values(); violate [CHECKLIST ITEM #10] and [#2]; use Colors.values()[randomIndex].name() directly without the extra variable.
    Additionally, there are redundant blank lines in Ball.java that should be removed according to [CHECKLIST ITEM #1], though those are minor once the main issues are fixed. Your structure and logic are solid, so after you address these checklist-related points, 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

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment on lines +5 to +7
System.out.println(Lottery.getRandomBall());
System.out.println(Lottery.getRandomBall());
System.out.println(Lottery.getRandomBall());

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This repeats three direct calls and doesn't use a loop. This violates checklist item #8: "Use for loop for creating several objects of the same class (e.g., when number of objects is 3)." Consider using a constant for the count (e.g. NUMBER_OF_BALLS = 3) and a for-loop to create/print the balls.

Comment on lines +5 to +7
System.out.println(Lottery.getRandomBall());
System.out.println(Lottery.getRandomBall());
System.out.println(Lottery.getRandomBall());

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This calls a static method Lottery.getRandomBall() which is at odds with checklist item #3: "Don't use static methods in your solution. “Static methods are in general a bad practice. Let's better create an instance of a class which method you want to call.”" Prefer creating a Lottery instance (e.g. Lottery lottery = new Lottery();) and calling an instance method inside the loop.

Comment on lines +2 to +12

public class Ball {

private String color;
private int number;

public Ball(String color, int number) {
this.color = color;
this.number = number;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This violates checklist item #1: "Please don't add redundant empty lines to your code. “We don't need them after class declaration or method signature.”" Remove the unnecessary blank lines at/around the following lines: 2 (after package), 4 (after class declaration), 7 (between fields and constructor) and 12 (after constructor).

public String getRandomColor() {
return null;

public static String getRandomColor() {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is declared static, which violates the checklist item #3: "Don't use static methods in your solution. “Static methods are in general a bad practice. Let's better create an instance of a class which method you want to call.”" Consider making getRandomColor() an instance method and calling it on a ColorSupplier instance.


public static String getRandomColor() {
Colors[] colors = Colors.values();
Random random = new Random();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Creating a local Random here 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 class-level field (e.g., private final Random random = new Random();) to reuse one instance.

return null;

public static String getRandomColor() {
Colors[] colors = Colors.values();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The temporary variable Colors[] colors = Colors.values(); can be avoided, which violates checklist item #2: "Don't create temporary variables if you can directly return value from the method." Consider inlining Colors.values() or returning directly from the expression after addressing the Random reuse and name() change.

import java.util.Random;

public class Lottery {
public static Ball getRandomBall() {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is declared static which violates the checklist item #3: "Don't use static methods in your solution. “Static methods are in general a bad practice. Let's better create an instance of a class which method you want to call.”" Make getRandomBall() an instance method of Lottery and call it via a Lottery instance from main.


public class Lottery {
public static Ball getRandomBall() {
Random randomNumber = new Random();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A new Random is created inside the method which 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 class-level field (e.g., private final Random random = new Random();).

public class Lottery {
public static Ball getRandomBall() {
Random randomNumber = new Random();
return new Ball(ColorSupplier.getRandomColor(), randomNumber.nextInt(101));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This uses a literal 101 in nextInt(101). That breaks checklist item #9: "All magic numbers in your code should be constants." Also the task states "the maximum possible number should be 100" — introduce a constant like private static final int MAX_BALL_NUMBER = 100 and use random.nextInt(MAX_BALL_NUMBER + 1) (or otherwise document inclusive/exclusive bounds).

public class Lottery {
public static Ball getRandomBall() {
Random randomNumber = new Random();
return new Ball(ColorSupplier.getRandomColor(), randomNumber.nextInt(101));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code calls ColorSupplier.getRandomColor() statically which also violates checklist item #3: "Don't use static methods in your solution..." After you make ColorSupplier.getRandomColor() an instance method, create and reuse a ColorSupplier instance in Lottery and call that instance method here instead of a static call.

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.

Review Summary

I am requesting changes because there are still a couple of checklist violations that are considered required, not just cosmetic. Functionally your solution works: all required classes are present, random colors and numbers are generated correctly, and three balls are printed from main.

Critical issues to fix:

  1. In src/main/java/core/basesyntax/Application.java, the literal 3 used as the number of balls in the loop is a magic number and must be replaced with a named constant (for example, private static final int NUMBER_OF_BALLS = 3; and then use NUMBER_OF_BALLS in the for loop). This is required by the checklist that all magic numbers be declared as constants.

Required style fixes:
2. In src/main/java/core/basesyntax/ColorSupplier.java, remove redundant empty lines: between package and import, before/after the Random field, and any trailing blank line at the end of the file so that class and method signatures are followed directly by code.
3. In src/main/java/core/basesyntax/Ball.java, remove the extra blank lines after the class declaration, before the constructor, and between the constructor and toString() as they violate the “no redundant empty lines after class/method signature” checklist rule.

Once you address the magic number and clean up those extra blank lines, your solution will fully meet both the functional and formatting requirements. You’re very close—your core implementation is solid, and these are straightforward final adjustments to align with the course standards.


✨ 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

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment on lines +3 to +9
import java.util.Random;

public class ColorSupplier {
private static final Random RANDOM = new Random();

public String getRandomColor() {
return null;
return Colors.values()[RANDOM.nextInt(Colors.values().length)].name();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file contains redundant empty lines. This violates the checklist item: "Please don't add redundant empty lines to your code. We don't need them after class declaration or method signature." Remove the unnecessary blank lines between package/import, before/after the field declaration, and at the end of the file so the class/method signatures are directly followed by code.

@@ -0,0 +1,19 @@
package core.basesyntax;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file has a redundant empty line here. This violates the checklist item: "Please don't add redundant empty lines to your code. We don't need them after class declaration or method signature." Remove the blank line.

public class Ball {
private final String color;
private final int number;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant empty line before the constructor — this violates the checklist: "Please don't add redundant empty lines to your code. We don't need them after class declaration or method signature." Remove the blank line.

this.color = color;
this.number = number;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant empty line between the constructor and the overridden method — this violates the checklist: "Please don't add redundant empty lines to your code. We don't need them after class declaration or method signature." Remove the blank line.


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line uses a literal 3 for the number of balls. This violates the checklist item: "All magic numbers in your code should be constants." Define a named constant (for example, private static final int NUMBER_OF_BALLS = 3;) at the class level and use it in the loop instead of the literal. See checklist: "All magic numbers in your code should be constants."

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.

Review Summary

I am approving your solution as an educational override so you can keep progressing while you practice these style details. Functionally, your classes (Ball, Colors, ColorSupplier, Lottery, and Application) meet the task requirements: random colors are generated from the enum, numbers are within the required range, and multiple balls are created and printed correctly.

There are a few checklist issues to fix next time: in Ball.java and ColorSupplier.java, remove redundant blank lines after the package, between import and class declarations, and around field/method signatures so code follows the style rule about no extra empty lines after signatures [CHECKLIST ITEM #1]. In Application.java, replace the local final int NM = 3; with a descriptive class-level constant like private static final int NUMBER_OF_BALLS = 3; and use that in the loop to avoid magic numbers [CHECKLIST ITEM #2]. In Lottery.java, rename MX to something descriptive such as MAX_NUMBER and consider marking randomNumber and colorSupplier as final to better express that they are not reassigned [CHECKLIST ITEM #3].

Overall, you’ve done well on the core logic; tightening up naming, constants, and formatting will bring your code fully in line with typical Java style and the project checklist.


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

Footnotes

  1. Rate AI review example

public class Ball {
private final String color;
private final int number;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This violates the checklist requirement: "Remove the extra blank lines after the class declaration, before the constructor, and between the constructor and toString() as they violate the ‘no redundant empty lines after class/method signature’ checklist rule." Remove the redundant blank line before the constructor so the field declarations are followed directly by the constructor signature.

this.color = color;
this.number = number;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This violates the checklist requirement: "Remove the extra blank lines after the class declaration, before the constructor, and between the constructor and toString() as they violate the ‘no redundant empty lines after class/method signature’ checklist rule." Remove the redundant blank line between the constructor and the toString() method so the constructor closing brace is followed directly by @Override and the method signature.

@@ -0,0 +1,19 @@
package core.basesyntax;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The checklist also asks to avoid redundant empty lines around class/method signatures (see: "Please don't add redundant empty lines to your code. We don't need them after class declaration or method signature."). Remove the blank line between the package statement and the public class Ball declaration so the file header and class declaration are adjacent.

@@ -1,7 +1,11 @@
package core.basesyntax;

import java.util.Random;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a redundant empty line between the package and import statements. This violates the checklist: "Remove redundant empty lines: between package and import, before/after the Random field, and any trailing blank line at the end of the file so that class and method signatures are followed directly by code." Remove the empty line so import immediately follows package.

package core.basesyntax;

import java.util.Random;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's an extra blank line between the import and the class declaration. This also falls under the same checklist rule: "Remove redundant empty lines: between package and import, before/after the Random field, and any trailing blank line at the end of the file so that class and method signatures are followed directly by code." Remove this blank line so the class declaration directly follows the imports.

import java.util.Random;

public class ColorSupplier {
private static final Random RANDOM = new Random();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Random field is fine, but the file currently has a blank line after it. Per the checklist: "Remove redundant empty lines: between package and import, before/after the Random field, and any trailing blank line at the end of the file so that class and method signatures are followed directly by code." Remove the blank line after the field so the next method signature follows immediately.


public class ColorSupplier {
private static final Random RANDOM = new Random();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is an extra blank line directly after the RANDOM field. Remove this blank line so the method getRandomColor() immediately follows the field, as required by the checklist: "Remove redundant empty lines: between package and import, before/after the Random field, and any trailing blank line at the end of the file so that class and method signatures are followed directly by code."


public static void main(String[] args) {
// create three balls using class Lottery and print information about them in console
final int NM = 3;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This violates the checklist requirement: "All magic numbers in your code should be constants." Specifically, the previous instruction also required: "The literal 3 used as the number of balls in the loop is a magic number and must be replaced with a named constant (for example, private static final int NUMBER_OF_BALLS = 3; and then use NUMBER_OF_BALLS in the for loop)." Replace final int NM = 3; with a descriptive class-level constant (e.g. private static final int NUMBER_OF_BALLS = 3;) and use it in the for loop.

import java.util.Random;

public class Lottery {
private static final int MX = 100;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This violates the checklist item: "Don't forget how to name constants according to style guide (the same with naming of enum values)". The constant MX is not descriptive — rename it to a clear, conventional constant name such as MAX_NUMBER or MAX_BALL_NUMBER (e.g. private static final int MAX_NUMBER = 100;).

Comment on lines +7 to +8
private Random randomNumber = new Random();
private ColorSupplier colorSupplier = new ColorSupplier();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider making these fields immutable to better express intent: private final Random randomNumber = new Random(); and private final ColorSupplier colorSupplier = new ColorSupplier();. This aligns with the checklist suggestion to think about which variables should be class-level and clarifies that these fields are not reassigned.

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