-
Notifications
You must be signed in to change notification settings - Fork 1.6k
add new classes and method #1987
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
Decision: REQUEST_CHANGES
Your solution is close, but there are a few critical requirement mismatches that block approval.
Critical issues to fix:
- ColorSupplier signature must be
public String getRandomColor()
and return enumname()
[e.g.,color.name()
], notColor
(ColorSupplier.java, lines ~5-10). This is required by the task and affects other classes. [CHECKLIST] - FigureSupplier must return
Figure
(notAbstractFigure
) ingetRandomFigure()
andgetDefaultFigure()
(FigureSupplier.java, lines ~9, ~24). The default figure must be a white circle with radius 10 (return typeFigure
). [Requirement] - Constructors and usage across figures should align with color as
String
if the supplier returnsString
(e.g., Rectangle/RightTriangle currently acceptColor
enums). Update constructors to acceptString color
and pass the string from ColorSupplier. [Requirement]
Minor but required formatting fixes:
- Remove leading/inner redundant empty lines across multiple files (e.g., AbstractFigure.java line 2; Circle.java lines 2,5,10,15; Drawable.java line 2; Figure.java line 2; Main.java lines 2, 7, 14; etc.). [CHECKLIST]
- Avoid magic numbers in FigureSupplier: introduce a
FIGURE_COUNT
constant instead ofrandom.nextInt(5)
and assign random dimensions to local variables before constructor calls for readability. [CHECKLIST] - The checklist advises against using abstract classes to set behavior; prefer interfaces for behavior. Either justify the abstract base as state-only or refactor to rely on
Figure
/Drawable
for behavior.
You’ve implemented areas and draw() correctly and structured the suppliers well—nice work. Address the above items and 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
@@ -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 the checklist guidance: "Don't begin class or method implementation with an empty line. Remove all redundant empty lines, be careful :)" Remove the empty line after the package declaration so the class declaration follows immediately. (Why: the checklist enforces no leading empty lines in class/method implementations.)
@@ -0,0 +1,13 @@ | |||
package core.basesyntax; | |||
|
|||
public abstract class AbstractFigure implements Drawable, 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 violates the checklist item: "Don't use abstract classes to set behavior for classes". Here AbstractFigure
is an abstract class that implements Drawable
and Figure
and holds behavior/state. According to the checklist you should prefer interfaces to express behavior; please either move behavior into interfaces or justify/update the design so it matches the checklist requirements.
|
||
public class Circle extends AbstractFigure { | ||
private final double radius; | ||
|
||
public Circle(Color color, double radius) { | ||
super(color); | ||
this.radius = radius; | ||
} | ||
|
||
@Override | ||
public double getArea() { | ||
return Math.PI * 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 checklist item #1: "Don't begin class or method implementation with an empty line. Remove all redundant empty lines, be careful :)" There are several redundant empty lines in this file (for example lines 2, 5, 10, 15). Please remove those empty lines so class and method implementations start immediately without a preceding blank line.
@@ -0,0 +1,21 @@ | |||
package core.basesyntax; | |||
|
|||
public class Circle extends AbstractFigure { |
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 file uses an abstract base class (AbstractFigure
) as the class that provides behavior/state (the class declaration public class Circle extends AbstractFigure
). This is a violation of checklist item #2: "Don't use abstract classes to set behavior for classes". Rework the design to prefer interfaces for behaviors (e.g., keep Drawable
and Figure
interfaces and avoid moving behavior into an abstract class), or provide a justification if the abstract class is strictly necessary for shared state only.
@@ -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: "Don't begin class or method implementation with an empty line. Remove all redundant empty lines, be careful :)" Remove the redundant blank line between the package statement and the enum declaration so the file does not start the class implementation with an empty line.
@@ -0,0 +1,24 @@ | |||
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. Remove all redundant empty lines, be careful :)" — there is a redundant empty line at the top of the file before the class declaration. Remove the blank line so the class starts immediately.
@@ -0,0 +1,24 @@ | |||
package core.basesyntax; | |||
|
|||
public class RightTriangle extends AbstractFigure { |
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 #2: "Don't use abstract classes to set behavior for classes" — the class extends AbstractFigure
(public class RightTriangle extends AbstractFigure {
). The checklist expects preferring interfaces for behavior (e.g., implement Figure
and Drawable
) rather than relying on an abstract class to provide behavior. Consider refactoring so shared behavior/state is provided via interfaces or adjusting the design to comply with the checklist.
private final double firstLeg; | ||
private final double secondLeg; | ||
|
||
public RightTriangle(Color color, double firstLeg, double 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 conflicts with the task description requirement: "ColorSupplier with public String getRandomColor()
method - for generating random color". The constructor currently accepts a Color
enum (public RightTriangle(Color color, double firstLeg, double secondLeg) {
), which is inconsistent with a supplier that should provide color as a String
. Align the figure constructors and AbstractFigure
(or its replacement) to accept a String
color (or adjust the ColorSupplier
to return a String
using name()
), so color handling matches the required supplier contract.
|
||
public class Square extends AbstractFigure { | ||
private final double side; | ||
|
||
public Square(Color color, double side) { | ||
super(color); | ||
this.side = side; | ||
} | ||
|
||
@Override | ||
public double getArea() { | ||
return 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 :)" — there are redundant empty lines in this file (for example at lines 2, 5, 10 and 15). Remove these blank lines so class and method implementations don't start with an empty line.
@@ -0,0 +1,21 @@ | |||
package core.basesyntax; | |||
|
|||
public class Square extends AbstractFigure { |
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 #2: "Don't use abstract classes to set behavior for classes". The class extends AbstractFigure
(public class Square extends AbstractFigure
) which centralizes behavior/state in an abstract class. The checklist asks to prefer interfaces for behavior; consider refactoring so behavior (e.g., getArea()
and draw()
) is provided by interfaces (Figure
, Drawable
) and avoid putting shared behavior into an abstract class unless there's a strong justification.
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 appreciate the solid functional implementation: figures compute areas correctly, draw() prints required info, suppliers and main() wiring are in place. However, there are a few checklist violations that are considered critical for this task’s acceptance.
- Critical: Multiple files begin class/method implementations with empty lines, violating the style checklist. Please remove redundant blank lines at the indicated locations, for example: Circle.java (lines 2, 5, 10, 15), Color.java (line 2), ColorSupplier.java (lines 2, 4, 7), Drawable.java (line 2), Figure.java (leading and between members), InfoArea.java (line 2), IsoscelesTrapezoid.java (leading and before constructor/getArea()/draw()), Main.java (lines 2, 7, 14), Rectangle.java (top, before constructor, before getArea(), before draw()), RightTriangle.java (lines 2, 6, 12, 17), Square.java (top, lines 5, 10, 15).
- Critical: FigureSupplier uses magic numbers. Replace
random.nextInt(5)
with a named constant (e.g.,FIGURE_TYPE_COUNT
) and use a constant for the default circle radius (e.g.,DEFAULT_RADIUS = 10
). This adheres to the “no magic numbers” rule. - Critical: In FigureSupplier, don’t pass expressions directly to constructors (e.g.,
random.nextInt(MAX_SIZE) + 1
). Assign them to local variables first (e.g.,double side = random.nextInt(MAX_SIZE) + 1;
) and then pass the variables for readability and checklist compliance.
Minor: Consider replacing other inline literals (like the array size 6
in Main) with constants for consistency.
Once these checklist items are addressed, your solution should be ready to approve. Good work on the core functionality—just tighten up the formatting and constants to meet the standards.
✨ 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,21 @@ | |||
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 file contains a leading empty line after the package declaration. 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 redundant empty line at line 2.
|
||
public class Circle extends Figure { | ||
private final double 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.
There is an empty line before the constructor. 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 redundant empty line at line 5.
super(color); | ||
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.
There is an empty line before the getArea() method. 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 redundant empty line at line 10.
public double getArea() { | ||
return Math.PI * 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.
There is an empty line before the draw() method. 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 redundant empty line at line 15.
@@ -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 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 between the package
declaration and the enum
declaration — remove the empty line at line 2 so the enum begins immediately after the package statement.
|
||
public class RightTriangle extends Figure { | ||
private final double firstLeg; | ||
private final double secondLeg; | ||
|
||
public RightTriangle(String color, double firstLeg, double secondLeg) { | ||
super(color); | ||
this.firstLeg = firstLeg; | ||
this.secondLeg = secondLeg; | ||
} | ||
|
||
@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.
This file contains redundant empty lines that violate the checklist requirement: "Don't begin class or method implementation with an empty line. Remove all redundant empty lines, be careful :)" Remove the extra blank lines so the class and its methods don't start or separate blocks with empty lines. Affected blank lines are at lines 2, 6, 12 and 17.
@@ -0,0 +1,21 @@ | |||
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 :)" Remove the blank line at the top of the file so the file does not begin with an empty line.
|
||
public class Square extends Figure { | ||
private final double 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 the checklist item: "Don't begin class or method implementation with an empty line. Remove all redundant empty lines, be careful :)" Remove the redundant empty line before the constructor (line 5).
super(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.
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 redundant empty line before the getArea() method (line 10).
public double getArea() { | ||
return 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 the checklist item: "Don't begin class or method implementation with an empty line. Remove all redundant empty lines, be careful :)" Remove the redundant empty line before the draw() method (line 15).
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 approval to support your progress. Functionality looks solid: figures compute areas correctly, draw()
outputs are clear, and suppliers/Main meet the task requirements. The main improvements needed are style-related: multiple files contain redundant leading blank lines that violate [CHECKLIST ITEM #1].
- Specific fixes (examples):
Circle.java
: remove the blank line afterpackage
(line 2), before the constructor, beforegetArea()
, and beforedraw()
.Color.java
: remove the blank line at line 2 so the enum follows the package line.Drawable.java
,InfoArea.java
,Figure.java
,Square.java
,Rectangle.java
,RightTriangle.java
,IsoscelesTrapezoid.java
: delete the leading blank line after the package and any blank line before methods/constructors.ColorSupplier.java
,FigureSupplier.java
,Main.java
: remove extra blank lines at the top and within the class (e.g., between imports and class, before methods, and between logical blocks).
Why this matters: starting class or method bodies with empty lines reduces readability and breaks the style requirements. How to fix: scan each file and ensure no blank line directly follows package
, and that constructors/methods start immediately after fields or annotations. Great job on the OOP design and correctness—once these small formatting tweaks are applied, your solution will be fully compliant. Keep it up!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
@@ -0,0 +1,21 @@ | |||
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. Remove all redundant empty lines, be careful :)". Remove the blank line at the top of the file (line 2) so the class declaration immediately follows the package statement.
|
||
public class Circle extends Figure { | ||
private final double 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 checklist item #1: "Don't begin class or method implementation with an empty line. Remove all redundant empty lines, be careful :)". Remove the blank line before the constructor so the constructor starts immediately after the field declarations.
super(color); | ||
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 checklist item #1: "Don't begin class or method implementation with an empty line. Remove all redundant empty lines, be careful :)". Remove the blank line before the getArea() method so the method starts without a preceding empty line.
public double getArea() { | ||
return Math.PI * 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 checklist item #1: "Don't begin class or method implementation with an empty line. Remove all redundant empty lines, be careful :)". Remove the blank line before the draw() method so the method starts immediately after the annotation/previous method.
@@ -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: "Don't begin class or method implementation with an empty line. Remove all redundant empty lines, be careful :)" Remove the blank line at line 2 so the enum declaration follows the package declaration without an empty line.
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.
This blank line after the getArea() method is redundant. It violates the checklist item: "Don't begin class or method implementation with an empty line. Remove all redundant empty lines, be careful :)" Remove this blank line before the @OverRide of draw().
@@ -0,0 +1,21 @@ | |||
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 file contains a redundant empty line. 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 the blank line between the package declaration and the class declaration.
|
||
public class Square extends Figure { | ||
private final double 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 file contains a redundant empty line inside the class. 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 the blank line before the constructor.
super(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.
This file contains a redundant empty line inside the class. 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 the blank line before the overridden getArea() method.
public double getArea() { | ||
return 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 file contains a redundant empty line inside the class. 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 the blank line before the overridden draw() method.
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
The test task has already been approved.
[CODE: AAP]
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.
No description provided.