-
Notifications
You must be signed in to change notification settings - Fork 1.6k
added classes and methods according to the task #1965
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?
added classes and methods according to the task #1965
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
I’m requesting changes because several checklist requirements are violated, which are considered critical for this task’s acceptance, even though functionality works.
Critical issues to fix (blockers):
- Magic numbers/constants (FigureSupplier, Main): Replace literals like
5
,10
,"white"
, and array size6
with namedstatic final
constants (e.g.,FIGURE_TYPE_COUNT
,MAX_RANDOM_DIMENSION
,DEFAULT_RADIUS
,DEFAULT_COLOR
,FIGURE_ARRAY_SIZE
). This is required by the checklist. - Expressions passed directly to constructors (FigureSupplier): Don’t pass
random.nextInt(...) + 1
inline. Compute into local variables first, then pass variables to constructors for readability as per checklist. - Non-English comments (Main, FigureSupplier): Replace Ukrainian comments with English (e.g.,
// first half - random
,// second half - default
,// draw all
,// five figure types
). Checklist requires English-only code/messages. - Constant naming (ColorSupplier): Rename
Colors
toCOLORS
and update usage toCOLORS[random.nextInt(COLORS.length)]
to follow UPPER_SNAKE_CASE forstatic final
constants. - Redundant empty lines across multiple files (AbstractFigure, Circle, ColorSupplier, Drawable, Figure, FigureSupplier, IsoscelesTrapezoid, Main, Rectangle, RightTriangle, Square): Remove leading/extra blank lines after package/imports and before method/constructor bodies to comply with “Don’t begin class or method with an empty line.”
What’s good:
- All figure classes compute areas correctly and
draw()
prints the required info in English usingSystem.out.println()
. - Suppliers are functional;
Main
uses a singleFigureSupplier
and fills/draws as required.
After applying the above fixes, your solution should fully meet both functional and checklist requirements. Keep up the good work—these are quick, focused edits to bring the code to standard.
✨ 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,9 @@ | |||
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: "Don't begin class or method implementation with an empty line. Remove all redundant empty lines, be careful :)" — remove the redundant empty line after the package declaration so the class declaration begins immediately after the package line.
|
||
public abstract class AbstractFigure implements Figure, Drawable { | ||
protected String 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 checklist item: "Don't begin class or method implementation with an empty line. Remove all redundant empty lines, be careful :)" — remove the empty line before the constructor so the method implementation does not start with a blank line.
@@ -0,0 +1,22 @@ | |||
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 has a redundant blank line immediately 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 empty line so the class declaration starts directly after the package line.
|
||
public class Circle extends AbstractFigure { | ||
private 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 unnecessary 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 this blank line so the constructor is not preceded by an empty line.
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 unnecessary 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 this blank line so the method starts immediately with its annotation/signature.
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 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 blank line between the end of getArea()
and the @Override
for 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 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 blank line between the package declaration and the class declaration — remove this blank line.
|
||
public class Square extends AbstractFigure { | ||
private 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 blank line between the field declaration and 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 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 blank line between the constructor and the 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 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 blank line between getArea() and the 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.
Review Summary
Decision: REQUEST_CHANGES
Your implementation meets the functional requirements very well: figures compute areas correctly, draw() outputs the required information in English, FigureSupplier and ColorSupplier behave as expected, and Main orchestrates creation and printing properly. However, there is a checklist violation that is considered critical for acceptance in this course: redundant leading/empty lines in multiple files.
Critical issues to fix (checklist: “Don’t begin class or method implementation with an empty line. Remove all redundant empty lines”):
- Remove empty lines immediately after package declarations and before constructors/methods.
- AbstractFigure.java: line 2 (after package), line 5 (before constructor).
- Circle.java: after package; before constructor; before
getArea()
; beforedraw()
. - ColorSupplier.java: line 2 (before import), line 4 (before class), line 10 (before
getRandomColor()
). - Drawable.java: after package; before method; after method.
- Figure.java: after package; inside interface before/after method.
- FigureSupplier.java: after package; before import/class; between constants and fields; before
getRandomFigure()
body; beforegetDefaultFigure()
. - IsoscelesTrapezoid.java: after package; before constructor; before
getArea()
; beforedraw()
. - Main.java: after package; before
main
method; before each loop. - Rectangle.java: after package; before constructor; before
getArea()
; beforedraw()
. - RightTriangle.java: after package; before constructor; before
getArea()
; beforedraw()
. - Square.java: after package; before constructor; before
getArea()
; beforedraw()
.
Why this matters: the checklist explicitly requires removing these redundant blank lines, and many mentors treat violations as blockers even if functionality is correct.
How to fix efficiently:
- Delete the noted blank lines so class and method declarations begin immediately after the relevant previous line (package/imports or previous member). Consider enabling a formatter (e.g., IntelliJ’s default, or add an .editorconfig) to enforce this automatically.
Once you remove these formatting issues, your solution should be fully compliant. Nice work on the overall design and correctness—this is a quick formatting pass away from approval.
✨ 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,9 @@ | |||
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: "Don't begin class or method implementation with an empty line. Remove all redundant empty lines, be careful :)" — there is an empty line immediately after the package declaration. Remove this blank line so the class declaration starts without a leading empty line.
|
||
public abstract class AbstractFigure implements Figure, Drawable { | ||
protected String 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 checklist item: "Don't begin class or method implementation with an empty line. Remove all redundant empty lines, be careful :)" — there is an unnecessary empty line between the field declaration and the constructor. Remove this blank line so the constructor begins directly after field declarations.
@@ -0,0 +1,22 @@ | |||
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 empty line after the package declaration so the class declaration starts immediately after the package line.
|
||
public class Circle extends AbstractFigure { | ||
private 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 the checklist item: "Don't begin class or method implementation with an empty line. Remove all redundant empty lines, be careful :)" Remove the empty line before the constructor so the constructor declaration follows the field declaration without an empty line.
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 the checklist item: "Don't begin class or method implementation with an empty line. Remove all redundant empty lines, be careful :)" Remove the empty line before the getArea() method so the method declaration begins immediately after the previous closing brace.
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 violates the checklist item: "Don't begin class or method implementation with an empty line. Remove all redundant empty lines, be careful :)" Remove the empty line before the draw() method so the method declaration begins immediately after the previous closing brace.
@@ -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 empty line after the package declaration so the class declaration starts immediately after the package line.
|
||
public class Square extends AbstractFigure { | ||
private 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 empty line before the constructor so the constructor declaration follows the field declaration without an empty line.
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 empty line before the getArea() method so the method begins immediately after the previous closing brace.
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 empty line before the draw() method so the method declaration begins immediately after the previous closing brace.
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: functionally, your OOP design and behavior are solid. You correctly separated responsibilities (Figure#getArea()
vs Drawable#draw()
), centralized randomness in FigureSupplier
, reused AbstractFigure
for shared color
, and printed clear, English outputs with dimensions and areas.
Critical issue to fix (Checkstyle, blocks CI):
- EmptyLineSeparator violations across many files (e.g., FigureSupplier.java lines near top-level declarations). Why: the style rules require blank lines after
package
, betweenimport
and class/interface, and between fields/constructors/methods. How to fix: add one blank line afterpackage core.basesyntax;
, one blank line after the import block, and blank lines separating fields, constructors, and methods. Then runmvn checkstyle:check
locally until zero errors. Example pattern:- package line
- [blank line]
- imports
- [blank line]
- class declaration
- fields
- [blank line]
- constructor
- [blank line]
- methods
Minor improvements (optional):
- Encapsulation: make
color
private inAbstractFigure
and exposeprotected String getColor()
to reduce direct field access. - In Main, avoid casting by using a common type that includes
draw()
if you refactor the supplier’s return type.
Great job on meeting the core requirements; a quick formatting pass will unblock CI and make this ready to merge.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
No description provided.