-
Notifications
You must be signed in to change notification settings - Fork 1.6k
implemented figures generation #1973
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
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.
Review Summary
Great job setting up the class structure for the figures! However, I'm requesting changes because several core functional requirements are missing, which prevents the program from working as intended.
The most critical issues are:
-
Missing Area Calculation: None of the figure classes (
Circle
,Square
,Rectangle
, etc.) implement a method to calculate their area. This is a primary requirement. You'll need to add a method likegetArea()
to each figure and also update thedraw()
method to print this area as part of its output. -
Incorrect Figure Properties: Several figures have properties that make area calculation impossible. For example:
Rectangle
needslength
andwidth
, not just adiagonal
.RightTriangle
needsfirstLeg
andsecondLeg
instead ofhypotenuse
.Circle
must be defined by itsradius
, notcircumference
.
-
FigureSupplier
Logic: ThegetRandomFigure()
method doesn't correctly generate figures that need more than one dimension (likeRectangle
). It also returnsnull
in itsdefault
case, which violates the project checklist (#12). Additionally,getDefaultFigure()
creates a circle with a circumference of 10, but the task requires a radius of 10. -
Application Flow: In
Application.java
, the task requires you to first populate the entire array with figures in one loop, and then iterate through the completed array to draw them in a second, separate loop.
Please address these points to meet the core requirements. You're on the right track, and I'm here to help if you have questions!
✨ 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 Application { | ||
private static final int ARRAY_FIGURE_LENGTH = 4; | ||
private static final FigureSupplier figureSupplier = new FigureSupplier(); |
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.
While this works, the task guidelines (specifically checklist items #9 and #10) encourage creating instances of helper classes like FigureSupplier
as local variables inside the method where they are used (in this case, main
). This helps avoid static state and promotes better object-oriented design.
Consider moving this instantiation inside the main
method.
for (int i = 0; i < ARRAY_FIGURE_LENGTH; i++) { | ||
if (i >= (ARRAY_FIGURE_LENGTH / 2)) { | ||
figures[i] = figureSupplier.getDefaultFigure(); | ||
figures[i].toDraw(); | ||
} else { | ||
figures[i] = figureSupplier.getRandomFigure(); | ||
figures[i].toDraw(); | ||
} | ||
} |
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 task description requires you to first populate the entire array with figures and then, after the array is fully generated, iterate through it to display the details of each figure.
Currently, you are creating and drawing a figure in the same step. You should have two separate loops:
- A loop to populate the
figures
array. - A second loop to iterate through the populated array and call the
toDraw()
method on each element.
@@ -0,0 +1,18 @@ | |||
package core.basesyntax; | |||
|
|||
public class Circle extends Figure { |
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.
This class is missing a crucial piece of behavior required by the task description. All figures must have a method to calculate and return their area. You should add a method like getArea()
here.
|
||
public class Circle extends Figure { | ||
private static final String FIGURE_NAME = "Circle"; | ||
private double circumFerence; |
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.
According to the task description, a Circle
should be defined by its radius
, not its circumference. This property should be named radius
to match the requirements. Also, there's a small typo in the variable name; it should be circumference
if you were to keep it.
public void toDraw() { | ||
System.out.println("Figure: " + FIGURE_NAME | ||
+ ", color: " + this.getColor() | ||
+ ", circumFerence: " + circumFerence); |
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 output format for the draw
method is incomplete. The task description requires printing the area of the figure as well. The example format is: Figure: <type>, area: <value> sq. units, <prop1>: <value> units, ...
. Please update this method to include the calculated area in the output.
public void toDraw() { | ||
System.out.println("Figure: " + FIGURE_NAME | ||
+ ", color: " + this.getColor() | ||
+ ", hypotenuse: " + hypotenuse); |
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 output format for the draw
method is incomplete. The task description requires printing the area of the figure as well. The example format is: Figure: <type>, area: <value> sq. units, <prop1>: <value> units, ...
. Please update this method to include the calculated area and all relevant properties in the output.
@@ -0,0 +1,19 @@ | |||
package core.basesyntax; | |||
|
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.
This violates checklist item #1: 'Don't begin class or method implementation with an empty line.' Please remove this redundant empty line.
@@ -0,0 +1,19 @@ | |||
package core.basesyntax; | |||
|
|||
public class Square extends Figure { |
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 task description requires that all figures must have a method to calculate and return their area. This class is missing that implementation. You need to add a method, for example public double getArea()
, that returns the area of the square.
|
||
public class Square extends Figure { | ||
private static final String FIGURE_NAME = "Square"; | ||
// Чи треба в кожному класі створювати одну і ту саму константу? |
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.
This violates checklist item #13: 'Use only eng in messages/code'. Please ensure all comments and output are in English to maintain code consistency.
public void toDraw() { | ||
System.out.println("Figure: " + FIGURE_NAME | ||
+ ", color: " + this.getColor() | ||
+ ", side: " + side); |
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 output format for the draw
method is incomplete. The task description requires printing the area of the figure as well. The example format is: Figure: <type>, area: <value> sq. units, <prop1>: <value> units, ...
. Please update this method to include the calculated area in the output.
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 Summary
Excellent progress on the overall structure! The figure classes and area calculations are well-implemented. However, I'm requesting changes because there are a few critical bugs that prevent the program from meeting the requirements.
Here are the main issues to address:
FigureSupplier
returnsnull
: InFigureSupplier.java
, thegetRandomFigure()
method returnsnull
in itsdefault
case, which will crash the program with aNullPointerException
. This violates a key requirement to never returnnull
.- Incorrect Random Figure Generation: The
getRandomFigure()
method generates a single random value and reuses it for all dimensions of a figure. This means, for example, that yourRectangle
will always be aSquare
. You need to generate a new, separate random number for each dimension. - Bug in
IsoscelesTrapezoid
: The constructor forIsoscelesTrapezoid
is missing the linethis.height = height;
, causing the area to always be calculated as zero.
Once those are fixed, please also review the output format to ensure it includes the required units (e.g., sq. units
and units
) as specified in the task description. You're very close to a complete solution!
✨ 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
private static final String FIGURE_NAME = "Circle"; | ||
private double radius; | ||
|
||
public Circle(String color, double circumFerence) { |
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 parameter name circumFerence
is misleading. The task requires the circle to be defined by its radius, and this value is being assigned to the radius
field inside the constructor. Please rename the parameter to radius
for clarity and to align with the task requirements.
public void toDraw() { | ||
System.out.println("Figure: " + FIGURE_NAME | ||
+ ", color: " + this.getColor() | ||
+ ", area: " + this.getArea() | ||
+ ", radius: " + radius); |
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 output format is missing the units specified in the task description. The example format is Figure: <type>, area: <area> sq. units, <prop1>: <val1> units, ...
. Please add " sq. units" after the area and " units" after the radius to match the required format.
|
||
public String getRandomColor() { | ||
int index = random.nextInt(colors.length); | ||
return colors[index].toString(); |
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.
This violates checklist item #14: "Use name() for getting String representation of enum constants".
The checklist advises against using .toString()
because it can be overridden, which might lead to unexpected behavior. The .name()
method is the standard and safer way to get the exact string representation of an enum constant. Please switch to using .name()
here.
package core.basesyntax; | ||
|
||
public interface FigureBehavior { | ||
void toDraw(); |
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.
While this is functionally correct, the method name toDraw()
is not idiomatic in Java. Method names should typically start with a verb that describes the action. A more conventional name would be draw()
.
Please consider renaming this method to draw()
here and in all the classes that implement it. This will make your code more readable and aligned with standard Java practices.
|
||
public Figure getRandomFigure() { | ||
int index = random.nextInt(figures.length); | ||
String nameOfFigure = figures[index].toString(); |
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.
This violates checklist item #14: "Use name() for getting String representation of enum constants".
The checklist explains that .toString()
can be overridden, making .name()
a safer and more reliable choice for getting the string value of an enum constant.
public IsoscelesTrapezoid(String color, double firstBase, double secondBase, double height) { | ||
super(color); | ||
this.firstBase = firstBase; | ||
this.secondBase = secondBase; | ||
} |
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.
There's a critical bug in this constructor. The height
parameter is accepted but is never assigned to the this.height
field. As a result, this.height
will always be its default value of 0.0
, and the getArea()
method will always return 0
.
You need to add the line this.height = height;
inside the constructor to fix this.
public void toDraw() { | ||
System.out.println("Figure: " + FIGURE_NAME | ||
+ ", color: " + this.getColor() | ||
+ ", area: " + this.getArea() | ||
+ ", firstBase: " + firstBase | ||
+ ", seconfBase: " + secondBase | ||
+ ", height: " + height); | ||
} |
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.
This method has a few issues:
- Method Name: As mentioned in other files, the method should be named
draw()
to follow standard conventions and the task description. - Output Format: The output is missing the units specified in the task description. You need to add " sq. units" after the area and " units" after each dimension (
firstBase
,secondBase
,height
). - Typo: There is a typo in the output string on line 26:
seconfBase
should besecondBase
.
public void toDraw() { | ||
System.out.println("Figure: " + FIGURE_NAME | ||
+ ", color: " + this.getColor() | ||
+ ", area: " + this.getArea() | ||
+ ", length: " + length | ||
+ ", width: " + width); | ||
} |
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.
This method has two issues that need to be addressed:
- Method Name: The name
toDraw()
is unconventional. In Java, methods that perform an action are typically named with a verb, sodraw()
would be a more standard and readable name. - Output Format: The task description specifies a clear output format that includes units. Please update the
println
statement to add " sq. units" after the area and " units" after the length and width values to match the requirement.
public void toDraw() { | ||
System.out.println("Figure: " + FIGURE_NAME | ||
+ ", color: " + this.getColor() | ||
+ ", area: " + this.getArea() | ||
+ ", firstLeg: " + firstLeg | ||
+ "secondLeg: " + secondLeg); | ||
} |
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.
This method has a few issues that prevent it from meeting the task requirements:
- Method Name: The method should be named
draw()
to follow common Java conventions for action-based methods. - Missing Units: The task description requires the output to include units, for example,
area: 12.5 sq. units
andfirstLeg: 7 units
. Please add" sq. units"
and" units"
to the output string. - Formatting Error: There is a missing comma and space in the string on line 25. It should be
+ ", secondLeg: " + secondLeg
instead of+ "secondLeg: " + secondLeg
. This will fix the output formatting.
public void toDraw() { | ||
System.out.println("Figure: " + FIGURE_NAME | ||
+ ", color: " + this.getColor() | ||
+ ", area: " + this.getArea() | ||
+ ", side: " + side); |
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 output format is missing the units specified in the task description. The example format is Figure: <type>, area: <area> sq. units, <prop1>: <val1> units, ...
.
Please add " sq. units" after the area and " units" after the side to match the required format.
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 Summary
Excellent work fixing the critical bugs! The program now runs correctly, which is a fantastic achievement. I'm approving this solution to help you move forward, but please review these suggestions to align your code with best practices for future tasks.
Here are a few key areas for refinement:
- In
Figure.java
, thegetArea()
method should be defined in its own separate interface to follow the interface segregation principle (checklist item #4). - Always use
.name()
instead of.toString()
for getting the string value of enums, as seen inColorSupplier.java
(checklist item #14). - Avoid "magic numbers" by declaring values like
4
inApplication.java
and10
inFigureSupplier.java
as named constants (checklist item #7). - For better readability, assign random values to variables before passing them into constructors in
FigureSupplier.java
(checklist item #8).
You've made great progress. Keep up the excellent work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
|
||
public class Application { | ||
public static void main(String[] args) { | ||
final int arrayLength = 4; |
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.
This is a "magic number". According to checklist item #7, all magic numbers should be declared as constants. It would be better to define this at the class level to give it a descriptive name.
For example:
public class Application {
private static final int FIGURES_ARRAY_LENGTH = 4;
public static void main(String[] args) {
Figure[] figures = new Figure[FIGURES_ARRAY_LENGTH];
// ...
}
}
private static final String FIGURE_NAME = "Circle"; | ||
private double radius; | ||
|
||
public Circle(String color, double circumFerence) { |
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 parameter name circumFerence
is a bit misleading here, as the value passed is used as the radius. To make the code clearer, it's a good practice to name parameters to reflect what they represent. In this case, renaming it to radius
would be more accurate.
|
||
public String getRandomColor() { | ||
int index = random.nextInt(colors.length); | ||
return colors[index].toString(); |
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.
This violates checklist item #14: "Use name() for getting String representation of enum constants".
While toString()
often works, it can be overridden. The name()
method is guaranteed to return the exact name of the enum constant as it's declared in the source code, which is the safer and required approach for this task.
return color; | ||
} | ||
|
||
abstract double 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.
This violates checklist item #4: "Don't put all behavior into a single interface if the methods are conceptually different from each other... the draw()
and getArea()
methods are not conceptually close to each other."
You've done a great job separating the draw()
method into the FigureBehavior
interface. To fully adhere to the requirement, the getArea()
method should also be defined in its own separate interface (e.g., AreaCalculable
). This would mean Figure
would not need to be an abstract class, and concrete figures would implement both interfaces.
|
||
public Figure getRandomFigure() { | ||
int index = random.nextInt(figures.length); | ||
String nameOfFigure = figures[index].toString(); |
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.
This violates checklist item #14: "Use name() for getting String representation of enum constants".
While toString()
often works, it's safer to use name()
because toString()
can be overridden. Using name()
ensures you always get the declared name of the enum constant.
|
||
switch (nameOfFigure) { | ||
case "Circle": | ||
return new Circle(randomColor, random.nextDouble()); |
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.
This violates checklist item #8: "Creating a figure, don't pass expressions in the constructor. Create separate variables and pass them on for better code readability."
Instead of passing random.nextDouble()
directly, you should assign it to a local variable first.
Example:
double radius = random.nextDouble();
return new Circle(randomColor, radius);
This rule applies to all the figure creation calls within this switch
block.
} | ||
|
||
public Figure getDefaultFigure() { | ||
return new Circle("White", 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.
This line contains two "magic numbers", which violates checklist item #7: "All magic numbers in your code should be constants."
- The color
"White"
should be a constant. A good practice would be to use theColors
enum you've already created, likeColors.WHITE.name()
. - The radius
10
should also be defined as a named constant, for example:private static final double DEFAULT_RADIUS = 10;
.
+ ", color: " + this.getColor() | ||
+ ", area: " + this.getArea() + " sq. units" | ||
+ ", firstBase: " + firstBase + " units" | ||
+ ", seconfBase: " + secondBase + " units" |
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.
There's a small typo here. It should be secondBase
to match the variable name and the logical property name.
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.
Good job! Let's improve your solution
figures[i] = figureSupplier.getRandomFigure(); | ||
} | ||
} | ||
|
||
for (Figure figure : figures) { | ||
figure.draw(); | ||
} |
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.
figures[i] = figureSupplier.getRandomFigure(); | |
} | |
} | |
for (Figure figure : figures) { | |
figure.draw(); | |
} | |
figures[i] = figureSupplier.getRandomFigure(); | |
} | |
figures[i].draw(); | |
} |
@@ -0,0 +1,10 @@ | |||
package core.basesyntax; | |||
|
|||
public enum Colors { |
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.
enum name should be singular according to the naming convention
public enum Colors { | |
public enum Color { |
@@ -0,0 +1,5 @@ | |||
package core.basesyntax; | |||
|
|||
public interface AreaCalculable { |
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.
public interface AreaCalculable { | |
public interface AreaCalculator { |
@@ -0,0 +1,5 @@ | |||
package core.basesyntax; | |||
|
|||
public interface FigureBehavior { |
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.
public interface FigureBehavior { | |
public interface Drawable { |
} | ||
|
||
public Figure getDefaultFigure() { | ||
return new Circle("White", DEFAULT_RADIUS); |
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.
use WHITE from enum
@@ -0,0 +1,9 @@ | |||
package core.basesyntax; | |||
|
|||
public enum 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.
public enum Figures { | |
public enum FigureName { |
package core.basesyntax; | ||
|
||
public enum Figures { | ||
Circle, |
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.
read how to name enum values
String randomColor = colorSupplier.getRandomColor(); | ||
|
||
switch (nameOfFigure) { | ||
case "Circle": |
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.
you can use your FigureName enum values
package core.basesyntax; | ||
|
||
public class Circle extends Figure { | ||
private static final String FIGURE_NAME = "Circle"; |
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.
you don't neeed constant if you have FigureName enum
No description provided.