add Figures with OOP #1773
add Figures with OOP #1773
Conversation
There was a problem hiding this comment.
Thank you for your hard work on this project! While your code meets many of the task requirements, there are a few critical issues that need to be addressed before we can approve the pull request. Specifically, returning null from the getRandomFigure() method is problematic and should be handled differently, such as by throwing an exception or returning a default figure. Additionally, using name() instead of toString() for enum constants is recommended for reliability. Addressing these issues will help ensure your code is robust and maintainable. Keep up the great work, and I look forward to your revisions! 😊
✨ 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.
| System.out.println("Figure: circle, area: " | ||
| + getArea() + " sq. units, radius: " | ||
| + this.radius + " units, color: " + this.color); |
There was a problem hiding this comment.
It's a good practice to use the getter method getColor() to access the color field instead of accessing it directly. This ensures better encapsulation and consistency with OOP principles.
| public String getRandomColor() { | ||
| int index = random.nextInt(Colors.values().length); | ||
| Colors randomColors = Colors.values()[index]; | ||
| return randomColors.toString(); |
There was a problem hiding this comment.
Use randomColors.name() instead of randomColors.toString() to get the string representation of the enum constant. This is a more reliable method as per the checklist recommendation.
| return new IsoscelesTrapezoid(getRandomColor(), random.nextInt(1, 10), | ||
| random.nextInt(1, 10), random.nextInt(1, 10)); | ||
| default: | ||
| return null; |
There was a problem hiding this comment.
Returning null from a method is a bad practice. Consider handling the default case differently, perhaps by throwing an exception or returning a default figure.
| + this.height + " units, color: " | ||
| + this.color); |
There was a problem hiding this comment.
Consider using getColor() instead of directly accessing the color field to maintain encapsulation and adhere to OOP principles.
| public void draw() { | ||
| System.out.println("Figure: rectangle, area: " | ||
| + getArea() + " sq. units, length: " | ||
| + this.length + ", width: " + this.width + " units, color: " + this.color); |
There was a problem hiding this comment.
Consider using getColor() instead of directly accessing the color field to maintain encapsulation and adhere to OOP principles.
| + this.secondLeg + " units, color: " | ||
| + this.color); |
There was a problem hiding this comment.
Consider using getColor() instead of directly accessing the color field to maintain encapsulation and adhere to OOP principles.
| public Square() { | ||
| } | ||
|
|
||
| public Square(String color, int site) { |
There was a problem hiding this comment.
Consider changing the parameter type of site from int to double to maintain consistency with other classes and allow for non-integer side lengths.
| public void draw() { | ||
| System.out.println("Figure: square, area: " | ||
| + getArea() + " sq. units, side: " | ||
| + this.site + " units, color: " + this.color); |
There was a problem hiding this comment.
Consider using getColor() instead of directly accessing the color field to maintain encapsulation and adhere to OOP principles.
There was a problem hiding this comment.
Great job on implementing the AreaCalculator, Circle, Colors, Drawable, Figure, Figures, IsoscelesTrapezoid, Rectangle, RightTriangle, and Square classes! 🎉 Your code is well-structured and meets the task requirements. However, there are a few areas for improvement:
- In the
FigureSupplierclass, usename()instead oftoString()for enum constants to ensure consistency. - Avoid using magic numbers by defining constants, such as for the number of figure types.
- Improve readability by assigning expressions to variables before passing them to constructors.
- Consider renaming the
sitevariable tosidein theSquareclass for clarity.
These changes will enhance the readability and maintainability of your code. Keep up the good work and continue learning from these suggestions! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| public String getRandomColor() { | ||
| int index = random.nextInt(Colors.values().length); | ||
| Colors randomColors = Colors.values()[index]; | ||
| return randomColors.toString(); |
There was a problem hiding this comment.
Use name() instead of toString() to get the string representation of enum constants. This ensures consistency even if toString() is overridden.
| } | ||
|
|
||
| public Figure getRandomFigure() { | ||
| int index = random.nextInt(Figures.values().length); |
There was a problem hiding this comment.
Consider defining a constant for the number of figure types to avoid using magic numbers. For example, private static final int FIGURE_COUNT = Figures.values().length;
| return new Circle(getRandomColor(), random.nextInt(1, 10)); | ||
| case Square: | ||
| return new Square(getRandomColor(), random.nextInt(1, 10)); | ||
| case Rectangle: | ||
| return new Rectangle(getRandomColor(), random.nextInt(1, 10), | ||
| random.nextInt(1, 10)); | ||
| case RightTriangle: | ||
| return new RightTriangle(getRandomColor(), random.nextInt(1, 10), | ||
| random.nextInt(1, 10)); | ||
| case IsoscelesTrapezoid: | ||
| return new IsoscelesTrapezoid(getRandomColor(), random.nextInt(1, 10), | ||
| random.nextInt(1, 10), random.nextInt(1, 10)); |
There was a problem hiding this comment.
Avoid passing expressions directly into constructors. Assign the random values to variables first, then pass those variables to the constructors for better readability.
|
|
||
| public class Square extends Figure { | ||
|
|
||
| private double site; |
There was a problem hiding this comment.
Consider renaming the site variable to side for clarity and consistency with the context of a square's side length.
No description provided.