-
Notifications
You must be signed in to change notification settings - Fork 1.6k
try#1 #1980
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?
try#1 #1980
Conversation
basiс task done #1
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
I’m requesting changes because there are several critical blockers that break requirements and can lead to incorrect results or runtime issues.
Critical issues to fix:
- FigureSupplier#getRandomFigure may return
null
(default -> null at line ~27), violating “don’t return null” and risking NPEs. Replace the default with a valid figure (e.g.,getDefaultFigure()
). [CHECKLIST ITEM #12] - IsoscelesTrapezoid area and height: area formula doesn’t use height and uses integer division (line ~15). Compute height with double math
(baseDown - baseUp) / 2.0
, validate the sqrt argument to avoid NaN, and set area as((baseUp + baseDown) / 2.0) * height
(line ~15). Also ensure inputs form a valid isosceles trapezoid. - RightTriangle area uses integer division
(base * leg) / 2
(line ~11). Use floating-point division(base * leg) / 2.0
for correctness. Also rename fields tofirstLeg
/secondLeg
to match the task’s public API and adjust draw() labels accordingly. - Magic numbers across Main and FigureSupplier (array size 6, random bounds 5 and 30, default radius 10, default color). Extract named constants and ensure random dimensions are strictly positive (use
nextInt(MAX) + 1
). [CHECKLIST ITEM #7] - Design requirement: avoid using an abstract class to set behavior. Move area behavior to a dedicated interface (e.g.,
Area
withgetArea()
), keepingDrawable
for draw().
Minor but required style fixes: remove redundant leading blank lines across multiple files (Circle, Drawable, Figure, IsoscelesTrapezoid, Rectangle, RightTriangle, Square), avoid inline constructor expressions in FigureSupplier by using local variables first, and fix Square’s draw() to print side: N units
(add missing space).
You’ve got the core structure right and many parts (Color/ColorSupplier/Main flow) are solid—addressing the above will make the solution fully correct and compliant. Keep going!
✨ 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
@@ -0,0 +1,25 @@ | |||
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 the checklist item: "Don't begin class or method implementation with an empty line. Remove all redundant empty lines, be careful :)" — there is an empty line between the package
statement and the public class Circle
declaration. Remove the blank line so the class declaration immediately follows the package. (File: Circle.java)
|
||
public class Circle extends Figure { | ||
private int 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.
This violates the checklist item: "Don't begin class or method implementation with an empty line. Remove all redundant empty lines, be careful :)" — the constructor public Circle(String color, int radius)
is preceded by an unnecessary blank line. Remove the blank line before the constructor. (File: Circle.java)
this.radius = radius; | ||
setArea(Math.PI * Math.pow(radius, 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.
This violates the checklist item: "Don't begin class or method implementation with an empty line. Remove all redundant empty lines, be careful :)" — the getRadius()
method is preceded by an unnecessary blank line. Remove the blank line before the method. (File: Circle.java)
public int getRadius() { | ||
return 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.
This violates the checklist item: "Don't begin class or method implementation with an empty line. Remove all redundant empty lines, be careful :)" — the setRadius(int radius)
method is preceded by an unnecessary blank line. Remove the blank line before the method. (File: Circle.java)
public void setRadius(int radius) { | ||
this.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.
This violates the checklist item: "Don't begin class or method implementation with an empty line. Remove all redundant empty lines, be careful :)" — there is an unnecessary blank line immediately before the @Override
annotation and draw()
method. Remove that blank line so the annotation/method begin without a preceding empty line. (File: Circle.java)
setColor(color); | ||
this.leg = leg; | ||
this.base = base; | ||
setArea((base * leg) / 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.
Functional correctness: the area calculation uses integer division: setArea((base * leg) / 2);
(line 11). Because base
and leg
are ints, (base * leg) / 2
does integer division and will lose the fractional .5 when applicable. Use floating-point arithmetic to preserve precision, e.g. setArea((base * leg) / 2.0);
or cast to double before division.
public class RightTriangle extends Figure { | ||
private int leg; | ||
private int base; |
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.
Task requirement mismatch: the task description lists triangle properties as firstLeg
and secondLeg
. The class currently uses base
and leg
fields and prints leg
/base
in draw(). Align field names and printed property names with the task specification (use firstLeg
and secondLeg
) so the class matches the required public API and printed output format.
public void draw() { | ||
System.out.println("Figure: " + "rightTriangle, " + "area: " + getArea() | ||
+ " sq. units, " + "leg: " + leg + " units, " + "base: " | ||
+ base + " units, " + "color: " + getColor()); |
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.
Print format consistency: draw()
prints "rightTriangle"
and uses property labels leg
/base
. After renaming fields to match the task (if you follow the previous comment), update the draw()
output to print the expected labels (e.g., firstLeg: ... units, secondLeg: ... units
) so the printed lines match the examples in the task description.
|
||
public class Square extends Figure { | ||
private int side; | ||
|
||
public Square(String color, int side) { | ||
setColor(color); | ||
this.side = side; | ||
setArea(Math.pow(side, 2)); | ||
} | ||
|
||
public int getSide() { | ||
return side; | ||
} | ||
|
||
public void setSide(int side) { | ||
this.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.
This violates checklist item #1: "Don't begin class or method implementation with an empty line. Remove all redundant empty lines, be careful :)" The file contains redundant empty lines before the class and before several methods. Remove the blank lines so declarations and method bodies start immediately. Offending lines include the blank lines at lines 2, 5, 11, 15 and 19.
@Override | ||
public void draw() { | ||
System.out.println("Figure: " + "square, " + "area: " + getArea() | ||
+ " sq. units, " + "side: " + side + "units, " + "color: " + getColor()); |
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.
Print-format correctness: The draw()
method prints "side: " + side + "units, "
(line 23) which yields e.g. side: 5units
(missing a space). The task requires that specific properties include units like side: 5 units
. Change the string to include a space before units
, e.g. "side: " + side + " units, "
. This ensures the printed output matches the required format in the task description.
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
I can’t approve yet because there are checklist-required fixes and a correctness risk that can break behavior.
Critical issues to fix:
- Style blockers [CHECKLIST ITEM #1]: redundant leading blank lines at the start of classes/methods across many files (e.g., Circle.java lines 2,5,10,14,18,23; Color.java line 2; ColorSupplier.java lines 2,4,7; Drawable.java line 2; Figure.java lines 2,5,9,13; FigureSupplier.java lines 2,4; Rectangle.java lines 2,6,12,16,20,24,28,33,40; RightTriangle.java lines 2,6,12,16,24,28,33,39; Square.java multiple spots; Main.java line 2). Remove those blank lines so declarations start immediately.
- Design/naming [CHECKLIST ITEM #2, #3]: Figure should not implement behavior interfaces; keep it for shared state only and let concrete figures implement
Drawable
and the area interface directly. RenameHaveArea
to a noun-based name (e.g.,Area
orAreaProvider
) and update usages. - Magic numbers/constants [CHECKLIST ITEM #7]: Replace literals like 5, 30, 10, 6 with named UPPER_SNAKE_CASE constants (e.g., FIGURE_TYPE_COUNT, MAX_DIMENSION, DEFAULT_RADIUS, DEFAULT_COLOR, FIGURE_COUNT) in FigureSupplier and Main.
- Constructor arguments [CHECKLIST ITEM #8]: Don’t pass expressions like
random.nextInt(30) + 1
directly to constructors; assign to local variables first and pass those variables. - IsoscelesTrapezoid correctness: validate inputs before
Math.sqrt(...)
to avoid NaN; ensure baseDown ≥ baseUp and leg is long enough. Keep state consistent by recomputing height in setters or computing height on demand, and removesetHeight
to prevent inconsistency.
Minor: Improve draw label for trapezoid to a human-friendly name (e.g., “isosceles trapezoid”).
You’ve implemented the core flow and figure behaviors well—once these checklist and correctness items are resolved, this will be ready to approve.
✨ 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 Circle extends Figure { | ||
private int radius; | ||
|
||
public Circle(String color, int radius) { | ||
setColor(color); | ||
this.radius = radius; | ||
} | ||
|
||
public int getRadius() { | ||
return radius; | ||
} | ||
|
||
public void setRadius(int radius) { | ||
this.radius = radius; | ||
} | ||
|
||
@Override | ||
public double getArea() { | ||
return (Math.PI * Math.pow(radius, 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.
This violates checklist item #1: "Do not begin class or method implementations with an empty line; remove redundant empty lines [CHECKLIST ITEM #1]."
There are redundant empty lines that begin the class or methods — please remove them so the class and each method definition start immediately (no blank line) at the following locations: line 2 (blank before class), line 5 (blank before constructor), line 10 (blank before getRadius), line 14 (blank before setRadius), line 18 (blank before getArea), and line 23 (blank before draw).
@@ -0,0 +1,13 @@ | |||
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: "Do not begin class or method implementations with an empty line; remove redundant empty lines [CHECKLIST ITEM #1]"
Remove the empty line at the top of the file (line 2) so the file begins immediately after the package declaration with the enum declaration.package ...
followed by public enum Color {
on the next line.
@@ -0,0 +1,12 @@ | |||
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: "Do not begin class or method implementations with an empty line; remove redundant empty lines [CHECKLIST ITEM #1]."
There is an unnecessary empty line at the top of the file (line 2). Remove the blank line between the package declaration and the import so the file doesn't start with a redundant empty line.
package core.basesyntax; | ||
|
||
import java.util.Random; | ||
|
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 class ColorSupplier { | ||
private final Random random = new Random(); | ||
|
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: "Do not begin class or method implementations with an empty line; remove redundant empty lines [CHECKLIST ITEM #1]."
There is an unnecessary empty line at line 7 immediately before the getRandomColor()
method. Remove this blank line so the method implementation does not begin with an empty line.
|
||
public class Square extends Figure { | ||
private int 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.
setColor(color); | ||
this.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.
public int getSide() { | ||
return 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.
public void setSide(int side) { | ||
this.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.
public double getArea() { | ||
return (Math.pow(side, 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.
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.
Sorry, the pull request review was unsuccessful for the following reasons:
Warning
No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
[CODE: NCM]
If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.
# Conflicts: # src/main/java/core/basesyntax/Figure.java # src/main/java/core/basesyntax/FigureSupplier.java # src/main/java/core/basesyntax/RightTriangle.java
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
Educational override: approving to help you progress while learning. Great job implementing all figures, printing draw info, and wiring Main to generate random and default figures correctly. Critical fixes to address next: 1) In Figure.java remove area-related state and getters/setters and keep only shared fields like color
[CHECKLIST ITEM]. 2) In FigureSupplier.java replace magic numbers like 5
, 30
, and default radius 10
with private static final
constants, avoid inline constructor expressions by storing random values in variables, ensure dimensions are positive (e.g., random.nextInt(MAX) + 1
), and never return null
in the switch default. 3) In IsoscelesTrapezoid.java validate constructor inputs before computing height and maintain invariants by either computing height on demand or recalculating it in setters; remove setHeight
to prevent inconsistency. Minor style fixes: remove redundant blank lines noted in Circle (line 14), Rectangle (line 26), RightTriangle (multiple spots), Square (line 18), Figure (several), FigureSupplier (lines 2, 4, 8, 12, 16, 30), and HaveArea/others [CHECKLIST ITEM]. Also, in ColorSupplier rename final int DEFAULT_COLOR
to a proper constant like private static final int COLOR_COUNT = Color.values().length
. You’re very close—tighten these details and your solution will be clean and robust!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
public void setRadius(int radius) { | ||
this.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.
This violates the checklist item: "Don't begin class or method implementation with an empty line. Remove all redundant empty lines, be careful :)" — there is a redundant empty line inside the class (line 14). Remove the blank line so methods and annotations are not separated by unnecessary empty lines.
import java.util.Random; | ||
public class ColorSupplier { | ||
private final Random random = new Random(); | ||
final int DEFAULT_COLOR = Color.values().length; |
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 the checklist item: "All magic numbers in your code should be constants." The field final int DEFAULT_COLOR = Color.values().length;
(line 5) should be declared as a proper constant (e.g., private static final int COLOR_COUNT = Color.values().length;
) and use an UPPER_SNAKE_CASE name. Making it static final
and private
will follow the checklist guidance and common practice.
@@ -0,0 +1,23 @@ | |||
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 the checklist item: "Don't begin class or method implementation with an empty line. Remove all redundant empty lines, be careful :)" Please remove this blank line so the class declaration starts immediately after the package statement.
public abstract class Figure implements Drawable { | ||
private String color; | ||
private double area; | ||
|
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 the checklist item: "Don't begin class or method implementation with an empty line. Remove all redundant empty lines, be careful :)" Please remove this redundant blank line inside the class.
public String getColor() { | ||
return 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.
This violates the checklist item: "Don't begin class or method implementation with an empty line. Remove all redundant empty lines, be careful :)" Please remove this redundant blank line.
public int getLeg() { | ||
return leg; | ||
} | ||
|
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 the checklist item: "Don't begin class or method implementation with an empty line. Remove all redundant empty lines, be careful :)" — remove the blank line before setLeg()
. (Line 16)
public void setLeg(int leg) { | ||
this.leg = leg; | ||
} | ||
|
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 the checklist item: "Don't begin class or method implementation with an empty line. Remove all redundant empty lines, be careful :)" — remove the blank line before getBase()
. (Line 20)
public int getBase() { | ||
return base; | ||
} | ||
|
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 the checklist item: "Don't begin class or method implementation with an empty line. Remove all redundant empty lines, be careful :)" — remove the blank line before setBase()
. (Line 24)
System.out.println("Figure: " + "rightTriangle, " + "area: " + getArea() | ||
+ " sq. units, " + "leg: " + leg + " units, " + "base: " | ||
+ base + " units, " + "color: " + getColor()); | ||
|
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 the checklist item: "Don't begin class or method implementation with an empty line. Remove all redundant empty lines, be careful :)" — remove the blank line inside the draw()
method block (after the println) so the method closing brace follows immediately. (Line 37)
public double getArea() { | ||
return (Math.pow(side, 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.
This violates the checklist item: "Don't begin class or method implementation with an empty line. Remove all redundant empty lines, be careful :)" — there is a redundant empty line inside the class (line 18). Remove that blank line so declarations/methods start immediately and the file matches the checklist requirement.
No description provided.