done 1.0 #1769
done 1.0 #1769
Conversation
There was a problem hiding this comment.
Thank you for your hard work on this project! While your code largely meets the task requirements, there are a few critical issues that need to be addressed before we can approve the pull request:
-
Redundant Fields: The
colorfield is redundantly declared in multiple classes (Circle,Square,RightTriangle,Rectangle,IsoscelesTrapezoid) even though it is already defined in theAbstractFigureclass. This redundancy can lead to confusion and potential errors, so it should be removed from these classes. -
Misspelled Variable: The variable
heigthis misspelled in several places. It should be corrected toheightto maintain code clarity and prevent potential bugs. -
Unreachable Code: In the
FigureSupplierclass, therandom.nextInt(4)call only generates numbers from 0 to 3, making the case4forRectangleunreachable. Adjust this torandom.nextInt(5)to ensure all cases are covered.
These issues impact the functionality and clarity of your code. Once these are resolved, your code will be in great shape! Keep up the great work, and I look forward to seeing the improvements. 😊
✨ 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.
| package core.basesyntax; | ||
|
|
||
| public class Circle extends AbstractFigure { | ||
| private String color; |
There was a problem hiding this comment.
The color field is redundant here because it is already defined in the AbstractFigure class. You should remove this field from the Circle class to avoid confusion and potential errors.
| Random random = new Random(); | ||
| ColorSupplier colorSupplier = new ColorSupplier(); | ||
| String color = colorSupplier.getRandomColor(); | ||
| int figureType = random.nextInt(4); |
There was a problem hiding this comment.
The random.nextInt(4) call will only generate numbers from 0 to 3. Therefore, the case 4 for Rectangle is unreachable. You should change random.nextInt(4) to random.nextInt(5) to include all cases.
| return new Circle(color,radius); | ||
| case 1: //RightTriangle | ||
| double base = 1 + random.nextInt(10); | ||
| double heigth = 1 + random.nextInt(10); |
There was a problem hiding this comment.
The variable heigth is misspelled. It should be height.
| case 3: //IsoscelesTrapezoid | ||
| double side1 = 1 + random.nextInt(10); | ||
| double side2 = 1 + random.nextInt(10); | ||
| heigth = 1 + random.nextInt(10); |
There was a problem hiding this comment.
The variable heigth is misspelled. It should be height.
| double radius = 1 + random.nextInt(10); | ||
| return new Circle(color,radius); | ||
| case 1: //RightTriangle | ||
| double base = 1 + random.nextInt(10); | ||
| double heigth = 1 + random.nextInt(10); | ||
| return new RightTriangle(color,base,heigth); | ||
| case 2: //Square | ||
| double side = 1 + random.nextInt(10); | ||
| return new Square(color,side); | ||
| case 3: //IsoscelesTrapezoid | ||
| double side1 = 1 + random.nextInt(10); | ||
| double side2 = 1 + random.nextInt(10); | ||
| heigth = 1 + random.nextInt(10); | ||
| return new IsoscelesTrapezoid(color,side1,side2,heigth); | ||
| case 4: //Rectangle | ||
| double length = 1 + random.nextInt(10); | ||
| double width = 1 + random.nextInt(10); | ||
| return new Rectangle(color,length,width); |
There was a problem hiding this comment.
Consider using constants for the magic numbers (e.g., 1 and 10) to improve code readability and maintainability.
| public class RightTriangle extends AbstractFigure { | ||
| private String color; | ||
| private double base; | ||
| private double heigth; |
There was a problem hiding this comment.
The variable heigth is misspelled. It should be height.
| public RightTriangle(String color,double base, double heigth) { | ||
| super(color); | ||
| this.base = base; | ||
| this.heigth = heigth; |
There was a problem hiding this comment.
The variable heigth is misspelled. It should be height.
| } | ||
|
|
||
| public double getArea() { | ||
| return 0.5 * base * heigth; |
There was a problem hiding this comment.
The variable heigth is misspelled. It should be height.
|
|
||
| @Override | ||
| public String describe() { | ||
| return "RightTriange " + getColor() + " base: " + base + " heigth: " |
There was a problem hiding this comment.
The variable heigth is misspelled. It should be height.
| package core.basesyntax; | ||
|
|
||
| public class Square extends AbstractFigure { | ||
| private String color; |
There was a problem hiding this comment.
The color field is redundant here because it is already defined in the AbstractFigure class. You should remove this field from the Square class to avoid confusion and potential errors.
There was a problem hiding this comment.
Great job on addressing most of the previous review comments! 🎉 Your implementation of the AbstractFigure class and its subclasses is well-structured, and you've successfully removed redundant fields, which is excellent. However, there are a couple of minor issues that need attention:
-
Misspelled Variable: The variable
heigthis still misspelled in theRightTriangleandIsoscelesTrapezoidclasses. Please correct it toheightto maintain consistency and prevent potential bugs. -
Code Functionality: The
FigureSupplierclass now correctly usesrandom.nextInt(5)to ensure all cases are reachable, which is a good improvement.
Your code largely meets the task requirements, and the decision to approve is made to help you move forward with your learning. Please review the comments and make the necessary corrections to improve your code. Keep up the good work, and continue learning from these experiences! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| return new Circle(color,radius); | ||
| case 1: //RightTriangle | ||
| double base = 1 + random.nextInt(10); | ||
| double heigth = 1 + random.nextInt(10); |
There was a problem hiding this comment.
The variable heigth is misspelled. It should be height to maintain code clarity and prevent potential bugs.
| case 3: //IsoscelesTrapezoid | ||
| double side1 = 1 + random.nextInt(10); | ||
| double side2 = 1 + random.nextInt(10); | ||
| heigth = 1 + random.nextInt(10); |
There was a problem hiding this comment.
The variable heigth is misspelled. It should be height to maintain code clarity and prevent potential bugs.
| public class IsoscelesTrapezoid extends AbstractFigure { | ||
| private double base1; | ||
| private double base2; | ||
| private double heigth; |
There was a problem hiding this comment.
The variable heigth is misspelled. It should be height to maintain code clarity and prevent potential bugs.
| super(color); | ||
| this.base1 = base1; | ||
| this.base2 = base2; | ||
| this.heigth = height; |
There was a problem hiding this comment.
The variable heigth is misspelled. It should be height to maintain code clarity and prevent potential bugs.
| } | ||
|
|
||
| public double getArea() { | ||
| return 0.5 * (base1 + base2) * heigth; |
There was a problem hiding this comment.
The variable heigth is misspelled. It should be height to maintain code clarity and prevent potential bugs.
| private double base; | ||
| private double heigth; | ||
|
|
||
| public RightTriangle(String color,double base, double heigth) { |
There was a problem hiding this comment.
The variable heigth is misspelled. It should be height to maintain code clarity and prevent potential bugs.
| public RightTriangle(String color,double base, double heigth) { | ||
| super(color); | ||
| this.base = base; | ||
| this.heigth = heigth; |
There was a problem hiding this comment.
The variable heigth is misspelled. It should be height to maintain code clarity and prevent potential bugs.
| } | ||
|
|
||
| public double getArea() { | ||
| return 0.5 * base * heigth; |
There was a problem hiding this comment.
The variable heigth is misspelled. It should be height to maintain code clarity and prevent potential bugs.
|
|
||
| @Override | ||
| public String describe() { | ||
| return "RightTriange " + getColor() + " base: " + base + " heigth: " |
There was a problem hiding this comment.
The variable heigth is misspelled. It should be height to maintain code clarity and prevent potential bugs.
| @Override | ||
| public String describe() { | ||
| return "RightTriange " + getColor() + " base: " + base + " heigth: " | ||
| + heigth + " area: " + getArea(); |
There was a problem hiding this comment.
The variable heigth is misspelled. It should be height to maintain code clarity and prevent potential bugs.
No description provided.