completed homework jv-lottery #1568
Conversation
There was a problem hiding this comment.
Great job on implementing the Color enum correctly! 👍 There are some suggestions for improving your Lottery class, such as using local variables, removing unnecessary methods, and reusing instances. These are minor issues that don't impact the approval of your Pull Request. Keep these tips in mind for future projects to write even more efficient and clean code! 😊👨💻
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
|
|
||
| public class Application { | ||
| public static void main(String[] args) { | ||
| // create three balls using class Lottery and print information about them in console |
There was a problem hiding this comment.
Please don't add redundant empty lines to your code. We don't need them after class declaration or method signature. You can remove the empty line at the start of the main method.
| public String getRandomColor() { | ||
| return null; | ||
| int index = new Random().nextInt(Color.values().length); | ||
| Color color = Color.values()[index]; | ||
| return color.toString(); | ||
| } |
There was a problem hiding this comment.
Don't create several instances of the same class if you can use only one instance for your purposes. In this case, a new Random object is created every time the getRandomColor method is called, which is unnecessary. Instead, you should create a single Random instance as a class field and reuse it.
| return null; | ||
| int index = new Random().nextInt(Color.values().length); | ||
| Color color = Color.values()[index]; | ||
| return color.toString(); |
There was a problem hiding this comment.
Use name() for getting String representation of enum constants. Instead of using color.toString(), you should use color.name() to ensure the correct string representation is returned, especially if toString() is overridden in the enum.
| import java.util.Random; | ||
|
|
||
| public class Lottery { | ||
| private Random rand = new Random(); |
There was a problem hiding this comment.
Avoid using class-level fields when local variables will suffice. The rand field can be made into a local variable within the getRandomBall method, as it does not need to be shared across multiple methods or instances.
| public Random getRand() { | ||
| return rand; | ||
| } | ||
|
|
||
| public void setRand(Random rand) { | ||
| this.rand = rand; | ||
| } |
There was a problem hiding this comment.
The getter and setter for rand are not necessary and should be removed. If the Random instance is moved to a local variable within getRandomBall, there would be no need for these methods.
| } | ||
|
|
||
| public Ball getRandomBall() { | ||
| return new Ball(new ColorSupplier().getRandomColor(), rand.nextInt(100)); |
There was a problem hiding this comment.
Don't create several instances of the same class if you can use only one instance for your purposes. Here, new ColorSupplier() creates a new ColorSupplier instance on every method call. It's better to create a single ColorSupplier instance and reuse it.
| } | ||
|
|
||
| public Ball getRandomBall() { | ||
| return new Ball(new ColorSupplier().getRandomColor(), rand.nextInt(100)); |
There was a problem hiding this comment.
Magic numbers should be avoided; consider defining a constant for the number 100 to give it meaning within the context of this method.
| public static void main(String[] args) { | ||
| // create three balls using class Lottery and print information about them in console | ||
| Lottery lottery = new Lottery(); | ||
| System.out.println(lottery.getRandomBall()); |
There was a problem hiding this comment.
What can you use to make the same action for n objects?
What if you need to print 100 balls, it would be 100 lines of code?
| public class ColorSupplier { | ||
| public String getRandomColor() { | ||
| return null; | ||
| int index = new Random().nextInt(Color.values().length); |
There was a problem hiding this comment.
Random should be class-level variable
| return null; | ||
| int index = new Random().nextInt(Color.values().length); | ||
| Color color = Color.values()[index]; | ||
| return color.toString(); |
| public Random getRand() { | ||
| return rand; | ||
| } | ||
|
|
||
| public void setRand(Random rand) { | ||
| this.rand = rand; | ||
| } |
There was a problem hiding this comment.
| public Random getRand() { | |
| return rand; | |
| } | |
| public void setRand(Random rand) { | |
| this.rand = rand; | |
| } |
| import java.util.Random; | ||
|
|
||
| public class Lottery { | ||
| private Random rand = new Random(); |
There was a problem hiding this comment.
| private Random rand = new Random(); | |
| private final Random random = new Random(); | |
| private final ColorSupplier colorSupplier = new ColorSupplier(); |
| } | ||
|
|
||
| public Ball getRandomBall() { | ||
| return new Ball(new ColorSupplier().getRandomColor(), rand.nextInt(100)); |
There was a problem hiding this comment.
100 is a magic number, create constant
| public String getColor() { | ||
| return color; | ||
| } | ||
|
|
||
| public void setColor(String color) { | ||
| this.color = color; | ||
| } | ||
|
|
||
| public int getNumber() { | ||
| return number; | ||
| } | ||
|
|
||
| public void setNumber(int number) { | ||
| this.number = number; | ||
| } |
There was a problem hiding this comment.
| public String getColor() { | |
| return color; | |
| } | |
| public void setColor(String color) { | |
| this.color = color; | |
| } | |
| public int getNumber() { | |
| return number; | |
| } | |
| public void setNumber(int number) { | |
| this.number = number; | |
| } |
You don't use these get and set methods
…y and Application
No description provided.