-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
getting random figures first update #1777
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your hard work on this project! 😊 While your implementation shows a good understanding of the task, there are a few critical issues that need to be addressed before we can approve the pull request:
-
FigureSupplier
Class: ThegetRandomFigure()
method should return a singleFigure
object, not an array. This is a core requirement of the task and needs to be corrected. -
Main
Class: Themain
method should create an array of figures where the first half are generated with random parameters and the second half have default parameters. This is not currently implemented as required. -
Figure
Class Design: TheFigure
class should not extendColorSupplier
. Instead, consider using composition to manage color, which would align better with object-oriented principles. -
Constructor Logic: Calling
getDraw()
within constructors is not ideal as it triggers drawing logic immediately upon object creation. This should be reviewed to ensure it matches your design intentions. -
Operator Precedence: Ensure correct operator precedence in area calculations to avoid integer division issues. Use parentheses to clarify the intended order of operations.
Please address these issues to align your code with the task requirements. Once these changes are made, your solution will be much stronger and ready for approval. Keep up the great work, and don't hesitate to reach out if you have any questions! 🚀
✨ 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 Circle() { | ||
super.name = "Circle"; | ||
super.color = "WHITE"; | ||
getDraw(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling getDraw()
in the constructor may not be ideal as it executes drawing logic immediately upon object creation. Consider whether this behavior is intended or if it should be triggered separately.
super.name = "Circle"; | ||
super.color = "WHITE"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assigning super.name
and super.color
directly in the constructor may limit flexibility if these properties are meant to be set dynamically. Ensure this aligns with your design intentions.
@@ -0,0 +1,7 @@ | |||
package core.basesyntax; | |||
|
|||
public class Figure extends ColorSupplier { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Figure
class extends ColorSupplier
, which may not be necessary unless you intend for Figure
to have direct access to color-supplying methods. Consider whether composition might be a better choice here.
public Figure[] getRandomFigure() { | ||
|
||
Figure [] figures = new Figure[]{new Square(random.nextInt(FIGURE_COUNT)), | ||
//square.color = getRandomColor(); | ||
//square.name = "Square "; | ||
//square.area = square.getArea(); | ||
|
||
new Rectangle(random.nextInt(FIGURE_COUNT), random.nextInt(FIGURE_COUNT)), | ||
//rectangle.color = getRandomColor(); | ||
//rectangle.name = "Rectangle "; | ||
//rectangle.area = rectangle.getArea(); | ||
|
||
new RightTriangle(random.nextInt(FIGURE_COUNT), random.nextInt(FIGURE_COUNT)), | ||
//rightTriangle.color = getRandomColor(); | ||
//rightTriangle.name = "Right Triangle "; | ||
//rightTriangle.area = rightTriangle.getArea(); | ||
|
||
new IsoscelesTrapezoid(random.nextInt(FIGURE_COUNT), | ||
random.nextInt(FIGURE_COUNT), random.nextInt(FIGURE_COUNT)) | ||
//isoscelesTrapezoid.color = getRandomColor(); | ||
//isoscelesTrapezoid.name = "Isosceles Trapezoid "; | ||
//isoscelesTrapezoid.area = isoscelesTrapezoid.getArea(); | ||
|
||
}; | ||
return figures; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The getRandomFigure
method should return a single Figure
instead of an array. Consider using a switch-case or similar logic to randomly select and return one figure.
Figure [] figures = new Figure[]{new Square(random.nextInt(FIGURE_COUNT)), | ||
//square.color = getRandomColor(); | ||
//square.name = "Square "; | ||
//square.area = square.getArea(); | ||
|
||
new Rectangle(random.nextInt(FIGURE_COUNT), random.nextInt(FIGURE_COUNT)), | ||
//rectangle.color = getRandomColor(); | ||
//rectangle.name = "Rectangle "; | ||
//rectangle.area = rectangle.getArea(); | ||
|
||
new RightTriangle(random.nextInt(FIGURE_COUNT), random.nextInt(FIGURE_COUNT)), | ||
//rightTriangle.color = getRandomColor(); | ||
//rightTriangle.name = "Right Triangle "; | ||
//rightTriangle.area = rightTriangle.getArea(); | ||
|
||
new IsoscelesTrapezoid(random.nextInt(FIGURE_COUNT), | ||
random.nextInt(FIGURE_COUNT), random.nextInt(FIGURE_COUNT)) | ||
//isoscelesTrapezoid.color = getRandomColor(); | ||
//isoscelesTrapezoid.name = "Isosceles Trapezoid "; | ||
//isoscelesTrapezoid.area = isoscelesTrapezoid.getArea(); | ||
|
||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The commented-out code for setting properties like color, name, and area should be reviewed. Ensure these properties are set correctly when creating figures.
|
||
|
||
FigureSupplier figureSupplier = new FigureSupplier(); | ||
Figure[] randomFigure = figureSupplier.getRandomFigure(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The getRandomFigure()
method should return a single Figure
instead of an array. Update this line to reflect the change in FigureSupplier
.
public Rectangle(int widthRectangle, int lengthRectangle) { | ||
super.name = "Rectangle"; | ||
super.color = getRandomColor(); | ||
this.widthRectangle = widthRectangle; | ||
this.lengthRectangle = lengthRectangle; | ||
getDraw(); | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling getDraw()
in the constructor may not be ideal as it executes drawing logic immediately upon object creation. Consider whether this behavior is intended or if it should be triggered separately.
public RightTriangle(int firstLeg, int secondLeg) { | ||
super.name = "Right Triangle"; | ||
super.color = getRandomColor(); | ||
this.firstLeg = firstLeg; | ||
this.secondLeg = secondLeg; | ||
getDraw(); | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling getDraw()
in the constructor may not be ideal as it executes drawing logic immediately upon object creation. Consider whether this behavior is intended or if it should be triggered separately.
@Override | ||
public double getArea() { | ||
|
||
return (firstLeg * secondLeg) / 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure correct operator precedence in the area calculation by using parentheses: return (firstLeg * secondLeg) / 2.0;
to avoid integer division issues.
public Square(int side) { | ||
super.name = "Square"; | ||
super.color = getRandomColor(); | ||
this.side = side; | ||
getDraw(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling getDraw()
in the constructor may not be ideal as it executes drawing logic immediately upon object creation. Consider whether this behavior is intended or if it should be triggered separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job on your submission! 🎉 While the implementation is mostly correct, there are a few areas for improvement to ensure your code meets the task requirements and enhances clarity:
-
Method Naming: Consider renaming the
getDraw()
method todraw()
across all relevant classes. This change will better reflect its purpose as an action method rather than a getter. -
Figure Generation Logic: In the
Main
class, ensure that the first half of the figures array is filled with figures having random parameters and the second half with default parameters. Adjust the logic accordingly. -
FigureSupplier Class:
- The
getRandomFigure()
method should generate a new random size for each figure to ensure variability. - Review and address any commented-out cases to ensure completeness and correctness.
- The
-
Constructor Logic: Ensure that
setName
andsetColor
methods are defined in theFigure
class to avoid potential compilation errors. -
Magic Numbers: Replace any magic numbers, such as
randomSize
, with constants for better readability and maintainability.
While these issues are important, the decision to approve your pull request is to encourage you to continue learning and improving. Please take the time to review the comments and make the necessary adjustments to enhance your code. Keep up the great work, and don't hesitate to reach out if you have any questions! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
super.setName("Circle"); | ||
super.setColor(color); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure that setName
and setColor
methods are defined in the Figure
class. If they are not, this will result in a compilation error.
} | ||
|
||
@Override | ||
public void getDraw() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider renaming getDraw()
to draw()
to better reflect its purpose, as it does not return a value but performs an action.
package core.basesyntax; | ||
|
||
public interface DrawFigures { | ||
void getDraw(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider renaming getDraw()
to draw()
to better reflect its purpose as a method that performs an action rather than returning a value.
|
||
private final Random random = new Random(); | ||
|
||
private final int randomSize = random.nextInt(10); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider generating a new random size within the getRandomFigure
method for each figure to ensure that each figure can have a unique size.
// case 3: return new Circle(colorSupplier.getRandomColor(), RANDOM_SIZE); | ||
// default: return new IsoscelesTrapezoid(colorSupplier.getRandomColor(), | ||
// RANDOM_SIZE, RANDOM_SIZE, RANDOM_SIZE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review the commented-out cases. If they are needed, ensure they are correctly implemented and uncommented. If not, consider removing them to clean up the code.
if (i < 3) { | ||
figures[i] = figureSupplier.getRandomFigure(i); | ||
} else { | ||
figures[i] = figureSupplier.getDefaultFigure(i); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic here should ensure that the first half of the figures array is filled with figures having random parameters and the second half with default parameters. Adjust the loop or conditions to meet this requirement.
} | ||
} | ||
for (Figure figure : figures) { | ||
figure.getDraw(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider renaming getDraw()
to draw()
to better reflect its purpose as a method that performs an action rather than returning a value.
} | ||
|
||
@Override | ||
public void getDraw() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider renaming getDraw()
to draw()
to better reflect its purpose as a method that performs an action rather than returning a value.
} | ||
|
||
@Override | ||
public void getDraw() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider renaming getDraw()
to draw()
to better reflect its purpose as a method that performs an action rather than returning a value.
} | ||
|
||
@Override | ||
public void getDraw() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider renaming getDraw()
to draw()
to better reflect its purpose as a method that performs an action rather than returning a value.
No description provided.