Skip to content

Added balls, random color, random number. #1998

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Papugo-FS wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Added balls, random color, random number. #1998

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d8729ad
Added balls, random color, random number.
Papugo-FS Jan 28, 2026
58dc597
small fix
Papugo-FS Jan 28, 2026
871e5e6
small fix
Papugo-FS Jan 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/main/java/core/basesyntax/Application.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

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++) {
Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor Jan 28, 2026

Choose a reason for hiding this comment

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

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.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor Jan 28, 2026

Choose a reason for hiding this comment

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

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.

System.out.println(new Lottery().getRandomBall());
Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor Jan 28, 2026

Choose a reason for hiding this comment

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

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.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor Jan 28, 2026

Choose a reason for hiding this comment

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

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.

}
}
}
30 changes: 30 additions & 0 deletions src/main/java/core/basesyntax/Ball.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
package core.basesyntax;

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

public String getColor() {
return this.color;
}

public void setColor(String color) {
this.color = color;
}

public int getNumber() {
return this.number;
}

public void setNumber(int number) {
this.number = number;
}

@Override
public String toString() {
return "Ball{ "
+ "color='" + getColor() + '\''
+ "number='" + getNumber() + '\''
+ '}';
}
}
6 changes: 5 additions & 1 deletion src/main/java/core/basesyntax/ColorSupplier.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
package core.basesyntax;

import java.util.Random;

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

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

@mateacademy-ai-mentor mateacademy-ai-mentor Jan 28, 2026

Choose a reason for hiding this comment

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

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.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor Jan 28, 2026

Choose a reason for hiding this comment

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

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.

}
}
10 changes: 10 additions & 0 deletions src/main/java/core/basesyntax/Colors.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
package core.basesyntax;

public enum Colors {
BLACK,
BLUE,
RED,
WHITE,
PURPLE,
PINK
}
24 changes: 24 additions & 0 deletions src/main/java/core/basesyntax/Lottery.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
package core.basesyntax;

import java.util.Random;

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

@mateacademy-ai-mentor mateacademy-ai-mentor Jan 28, 2026

Choose a reason for hiding this comment

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

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.


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

@mateacademy-ai-mentor mateacademy-ai-mentor Jan 28, 2026

Choose a reason for hiding this comment

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

This line has two issues:

  1. 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.

  2. 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.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor Jan 28, 2026

Choose a reason for hiding this comment

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

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.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor Jan 28, 2026

Choose a reason for hiding this comment

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

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.

ball.setColor(randomColor);
ball.setNumber(random.nextInt(FIGURE_COUNT));
return ball;
}

public Ball getBall() {
return ball;
}

public void setBall(Ball ball) {
this.ball = ball;
}
}