Hm jv lottery solution #1687
Hm jv lottery solution #1687
Conversation
There was a problem hiding this comment.
Great job on implementing the lottery application! 🎉 The code meets the core requirements and functions as expected. While there are a few minor improvements suggested, such as using name() instead of toString() for enums, avoiding inheritance misuse, defining magic numbers as constants, and specifying access modifiers, these do not critically impact the functionality. Keep up the good work and consider these suggestions for future enhancements! 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| public class ColorSupplier { | ||
| public String getRandomColor() { | ||
| return null; | ||
| int index = new Random().nextInt(Color.values().length); |
There was a problem hiding this comment.
It is better to create Random object on class level, because in current implementation you will create Random each time when method getRandomColor
| public class Lottery extends ColorSupplier { | ||
| public Ball getRandomBall() { | ||
| Random random = new Random(); | ||
| int value = random.nextInt(100); |
There was a problem hiding this comment.
Magic number, please create a constant
|
|
||
| import java.util.Random; | ||
|
|
||
| public class Lottery extends ColorSupplier { |
There was a problem hiding this comment.
There is not reason to extend lottery from ColorSupplier, they are 2 totally different classes with different purpose, so they should not be in one hierarchy. So I suggest you to just create and object of ColorSupplier in Lottery and use getRandomColor method
| @@ -2,6 +2,9 @@ | |||
|
|
|||
| public class Application { | |||
| public static void main(String[] args) { | |||
| // create three balls using class Lottery and print information about them in console | |||
| Lottery lottery = new Lottery(); | |||
| for (int i = 0; i < 3; i++) { | |||
There was a problem hiding this comment.
3 is also magic number, please move it to constant
| return null; | ||
| int index = random.nextInt(Color.values().length); | ||
| Color color = Color.values()[index]; | ||
| return color.toString(); |
There was a problem hiding this comment.
| private static final int MAX_BALL_NUMBER = 100; | ||
| private final Random random = new Random(); | ||
| private final ColorSupplier colorSupplier = new ColorSupplier(); | ||
| private final Ball ball = new Ball(); |
There was a problem hiding this comment.
| private final Ball ball = new Ball(); |
| int value = random.nextInt(MAX_BALL_NUMBER); | ||
| ball.setColor(colorSupplier.getRandomColor()); | ||
| ball.setNumber(value); |
There was a problem hiding this comment.
| int value = random.nextInt(MAX_BALL_NUMBER); | |
| ball.setColor(colorSupplier.getRandomColor()); | |
| ball.setNumber(value); | |
| Ball ball = new Ball; | |
| ball.setColor(colorSupplier.getRandomColor()); | |
| ball.setNumber(random.nextInt(MAX_BALL_NUMBER)); |
| @@ -1,7 +1,13 @@ | |||
| package core.basesyntax; | |||
|
|
|||
| public class Application { | |||
| public static final int TOTAL_BALLS = 3; | |||
There was a problem hiding this comment.
| public static final int TOTAL_BALLS = 3; | |
| private static final int TOTAL_BALLS = 3; |
… ColorSuppiler class. In the Application class, the access modifier in the TOTAL_BALLS field has been changed from public to private. Moved creation of the Ball object from the Lottery class level to the getRandomBall() method level
No description provided.