Classes created and tested #1725
Conversation
There was a problem hiding this comment.
Thank you for your hard work on this pull request! While your code is mostly well-implemented and meets many of the task requirements, there are a few critical issues that need to be addressed before we can approve it:
-
Static Method Usage: The
getRandomBallmethod in theLotteryclass should not rely on a static method approach by accepting parameters likecolorandrange. Instead, these should be generated internally, adhering to the task requirements. -
Access Modifiers: It's important to specify access modifiers for the
Lotteryclass and its methods to ensure proper encapsulation and maintainability. -
Magic Numbers: The use of magic numbers, such as the number range (100) and the number of balls to create (3), should be avoided. Defining them as constants will enhance code readability and maintainability.
-
Redundant Parameters: The
getRandomBallmethod should generate thecolorinternally using theColorSupplierclass, rather than accepting it as a parameter.
Please address these issues to ensure the code fully meets the task requirements. Once these changes are made, your code will be in great shape! Keep up the great work, and I'm confident you'll nail it with these adjustments! 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
🔄 Fix the issues, commit the changes, and then re-request my review using the Re-request review button in the Reviewers tab.
| public class Application { | ||
| public static void main(String[] args) { | ||
| // create three balls using class Lottery and print information about them in console | ||
| int ballsToCreate = 3; |
There was a problem hiding this comment.
Consider defining ballsToCreate as a constant, as it is a magic number. This will improve code readability and maintainability. For example, you can define it as private static final int BALLS_TO_CREATE = 3;.
| public class Application { | ||
| public static void main(String[] args) { | ||
| // create three balls using class Lottery and print information about them in console | ||
| int ballsToCreate = 3; | ||
| int numberRange = 100; |
There was a problem hiding this comment.
Similarly, numberRange should be defined as a constant to avoid using magic numbers. Define it as private static final int NUMBER_RANGE = 100;.
| public class ColorSupplier { | ||
| public String getRandomColor() { | ||
| return null; | ||
| int randomPosition = new Random().nextInt(Colors.values().length); | ||
| return Colors.values()[randomPosition].toString(); |
There was a problem hiding this comment.
Instead of using toString(), use the name() method to get the string representation of the enum constant. This is a more reliable approach as per the checklist guidelines.
There was a problem hiding this comment.
Thank you for your submission! 🎉 Your code demonstrates a solid understanding of the task requirements, and you've made good progress. However, there are a few critical issues that need to be addressed to ensure your code fully aligns with the task constraints:
-
getRandomBallMethod: This method should not accept parameters likecolorandrange. Instead, it should internally generate a random color using theColorSupplierclass and use a predefined constant for the range. This is crucial for adhering to the task requirements. -
Magic Numbers: Avoid using magic numbers such as
100for the number range. Define these as constants to enhance readability and maintainability. -
Naming Conventions: Ensure that constants are named using uppercase letters with underscores, such as
NUMBERandRANGE, to follow Java naming conventions. -
Access Modifiers: Specify access modifiers for your classes and methods to ensure proper encapsulation and code clarity.
Please review the comments and make the necessary improvements. Addressing these issues will help you avoid similar problems in the future and enhance your coding skills. Keep up the great work, and continue learning from your experiences! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| final int Number = 3; | ||
| final int Range = 100; |
There was a problem hiding this comment.
The constants Number and Range should be named using uppercase letters with underscores as separators, following Java naming conventions for constants. Consider renaming them to NUMBER and RANGE.
| Ball[] result = new Ball[Number]; | ||
|
|
||
| for (int i = 0; i < Number; i++) { | ||
| result[i] = lottery.getRandomBall(colorSupplier.getRandomColor(), Range); |
There was a problem hiding this comment.
The getRandomBall method should not accept parameters like color and range. Instead, it should internally generate these values. This is a critical issue as it does not adhere to the task requirements.
| import java.util.Random; | ||
|
|
||
| public class Lottery { | ||
| public Ball getRandomBall(String color, int range) { |
There was a problem hiding this comment.
The getRandomBall method should not accept color and range as parameters. Instead, it should internally generate a random color using the ColorSupplier class and use a predefined constant for the range. This change is necessary to adhere to the task requirements.
No description provided.