create lottery with a random balls #1945
Conversation
There was a problem hiding this comment.
Review Summary
Hi! Your solution correctly implements the main functionality, which is a great start. However, some changes are needed to meet all the project requirements and best practices outlined in the checklist.
Here are the key points to address:
RandomObject Usage: InColorSupplier.javaandLottery.java, a newRandomobject is created on every method call. Please declare it as aprivatefield and initialize it only once to improve efficiency [CHECKLIST ITEM #4, #5].- Magic Numbers: The number
100inLottery.javaand the count of balls inApplication.javashould be declared asprivate static finalconstants [CHECKLIST ITEM #9]. - Incorrect Random Range: The lottery number should be up to 100 inclusive.
nextInt(100)generates numbers from 0 to 99. You'll need to adjust the argument to include 100. - Missing
forLoop: TheApplicationclass should use aforloop to create the three balls instead of repeating the code [CHECKLIST ITEM #8].
Also, please remember to use .name() instead of .toString() for enums as required by the checklist [CHECKLIST ITEM #10].
Fixing these points will bring your code in line with the project's standards. Keep up the great work!
✨ 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
| public class ColorSupplier { | ||
| public String getRandomColor() { | ||
| return null; | ||
| int index = new Random().nextInt(Colours.values().length); |
| return null; | ||
| int index = new Random().nextInt(Colours.values().length); | ||
| Colours colour = Colours.values()[index]; | ||
| return colour.toString(); |
There was a problem hiding this comment.
This violates checklist item #10: 'Use name() for getting String representation of enum constants'. You should use .name() instead of .toString(). Additionally, the index and colour variables are temporary; you could combine these steps into a single return statement as suggested in checklist item #2.
|
|
||
| public class Ball { | ||
|
|
||
| private String colour; |
There was a problem hiding this comment.
The task description specifies that the field should be named color. While colour is a valid spelling, it's important to adhere to the exact naming conventions provided in the requirements.
| Ball firstBall = lottery.getRandomBall(); | ||
| Ball secondBall = lottery.getRandomBall(); | ||
| Ball thirdBall = lottery.getRandomBall(); | ||
| System.out.println(firstBall.toString()); | ||
| System.out.println(secondBall.toString()); | ||
| System.out.println(thirdBall.toString()); |
There was a problem hiding this comment.
This violates checklist item #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 and print the balls. This makes the code cleaner and easier to change.
Additionally, this violates checklist item #9: 'All magic numbers in your code should be constants'. The number of balls to create (3) should be defined as a constant and used as the loop's limit.
|
|
||
| public Ball getRandomBall() { | ||
| String randomColour = colorSupplier.getRandomColor(); | ||
| int randomNumber = new Random().nextInt(100); |
There was a problem hiding this comment.
This violates a few checklist items:
- Checklist item Wrote an assignment jv-lottery. #4: A new
Randominstance is created every time this method is called, which is inefficient. You should declareRandomas aprivatefield of theLotteryclass and reuse the same instance. - Checklist item Jv lottery #9: The number
100is a magic number. It should be declared as aprivate static finalconstant. - The requirement is that the maximum number is 100.
nextInt(100)generates numbers in the range[0, 99]. To include 100, you should usenextInt(101).
| String randomColour = colorSupplier.getRandomColor(); | ||
| int randomNumber = new Random().nextInt(100); | ||
| return new Ball(randomColour, randomNumber); |
There was a problem hiding this comment.
This violates checklist item #2: 'Don't create temporary variables if you can directly return value from the method'. You can call colorSupplier.getRandomColor() and the random number generation directly inside the Ball constructor to make the code more concise.
There was a problem hiding this comment.
Review Summary
Excellent work on this revision! You've successfully addressed all the feedback from the previous review. Your code is now clean, efficient, and fully compliant with the project requirements. I'm particularly impressed with how you've implemented the use of constants and ensured Random objects are instantiated correctly as class fields. Your solution is approved. Keep up the great work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
No description provided.