Skip to content

Conversation

@DAlex214
Copy link

No description provided.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a 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-driven architectural and correctness items are not met, which can affect grading and expected output.

Critical issues to fix:

  • Architecture: You use an abstract Figure with behavioral methods (getArea(), draw()). The checklist requires defining behaviors via separate interfaces and not using abstract classes to set behavior. Refactor by introducing two interfaces (e.g., HasArea with double getArea() and Drawable with void draw()), and have all figures implement them. Update or remove the abstract methods from Figure accordingly. Files: Figure.java, and all figure classes (Square, Rectangle, RightTriangle, Circle, IsoscelesTrapezoid).
  • Output correctness: In Circle.draw() the units label is a typo: "aq. units" → should be "sq. units" to match task examples and requirements. File: src/main/java/core/basesyntax/Circle.java (line where the area is printed).
  • Magic numbers: Replace hard-coded numeric/string literals with constants. Examples: random bound 10 and default radius/color in FigureSupplier (introduce MAX_DIMENSION, DEFAULT_RADIUS, DEFAULT_COLOR), and the array size 6 in HelloWorld (e.g., FIGURES_ARRAY_SIZE). Files: FigureSupplier.java, HelloWorld.java.
  • Constructor arguments readability: In FigureSupplier.getRandomFigure(), don’t pass expressions directly into constructors (e.g., new Square(color, random.nextInt(10) + 1)). First assign values to local variables, then pass them. File: FigureSupplier.java.

Minor but required style fixes:

  • Remove redundant empty lines that begin classes/methods across multiple files (e.g., Circle.java lines 2, 5, 10, 15; IsoscelesTrapezoid.java lines 2, 7, 14, 19; Rectangle.java lines 2, 6, 12, 17; RightTriangle.java lines 2, 6, 12, 17; Figure.java has an empty line after the package; HelloWorld.java lines 6, 12, 16, 20, 26).
  • ColorSupplier: reuse a class-level Random instead of creating a new one on each call. Add private final Random random = new Random(); and use it in getRandomColor().

Your functional logic and area formulas look good, and the main flow meets the task intent. Once you address the architectural refactor, fix the Circle units typo, extract constants, and clean up the empty lines, this should be ready to approve. Keep up the good work!


✨ 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

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

@@ -0,0 +1,22 @@
package core.basesyntax;

public class Circle extends Figure {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class currently extends an abstract Figure. The project checklist explicitly advises against using abstract classes to set behavior: "Don't use abstract classes to set behavior for classes". Prefer splitting behaviors into interfaces (for example, an interface for area calculation and another for drawing) and have figure classes implement those interfaces instead of relying on an abstract class for behavior. See checklist guidance: "Don't use abstract classes to set behavior for classes" and "Don't put all behavior into a single interface if the methods are conceptually different from each other." . Also note Figure.java in the project is abstract (dependency) which demonstrates the current design choice .

Comment on lines 18 to 20
System.out.println("Figure: circle, area: " + getArea()
+ " aq. units, radius: " + radius
+ " units, color: " + color);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The draw() output prints the area units as "aq. units" which appears to be a typo and doesn't match the task examples that use "sq. units". The task requires draw() to print all required info including area and units: "Each figure must provide a draw behavior that prints all information about the figure using System.out.println(), including: - Figure type name - Area value - All figure-specific properties with units where applicable - Color". Please change "aq. units" to "sq. units" so output matches the requirement/examples.

Comment on lines 2 to 15

public class Circle extends Figure {
private final double radius;

public Circle(String color, double radius) {
super(color);
this.radius = radius;
}

@Override
public double getArea() {
return Math.PI * radius * radius;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are redundant empty lines at the top of the file and before method declarations. The checklist states: "Don't begin class or method implementation with an empty line. Remove all redundant empty lines, be careful :)" Please remove these blank lines to comply with the style rule. Examples of blank lines here: line 2, line 5, line 10, line 15.

private static final String[] COLORS = {"red", "blue", "green", "yellow", "black"};

public String getRandomColor() {
Random random = new Random();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This creates a new Random on every call. Consider using a class-level Random instance instead and reusing it to avoid repeated allocations and to follow the checklist guidance: "Think about which variables should be local in the method and which should be class-level" . Example approach: add private final Random random = new Random(); as a field and use it inside getRandomColor() rather than new Random() here.

Comment on lines 3 to 12
public abstract class Figure {
protected String color;

public Figure(String color) {
this.color = color;
}

public abstract double getArea();

public abstract void draw();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file declares Figure as an abstract class that defines abstract behavioral methods getArea() and draw(). That design violates the checklist guidance: "Don't use abstract classes to set behavior for classes". The task requires separating behavior into interfaces rather than placing behavioral contracts in an abstract class. Consider moving getArea() and draw() into two separate interfaces (e.g., HasArea / AreaCalculator and Drawable) and let concrete figure classes implement them. You can keep a simple (non-abstract) Figure class only to hold shared state like color, or let each concrete class have its own color field.

public class RightTriangle extends Figure {
private final double firstLeg;
private final double secondLeg;

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 (line 6). The checklist requires removing redundant empty lines: "Don't begin class or method implementation with an empty line. Remove all redundant empty lines, be careful :)" Please remove this blank line to comply with style rules.

this.firstLeg = firstLeg;
this.secondLeg = secondLeg;
}

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 @Override of getArea() (line 12). Remove this redundant empty line as requested by the checklist: "Don't begin class or method implementation with an empty line. Remove all redundant empty lines, be careful :)"

public double getArea() {
return (firstLeg * secondLeg) / 2;
}

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 @Override of draw() (line 17). Remove this redundant blank line to satisfy the checklist style rule: "Don't begin class or method implementation with an empty line. Remove all redundant empty lines, be careful :)"

@@ -0,0 +1,23 @@
package core.basesyntax;

public class Square extends Figure {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class extends an abstract Figure. The checklist explicitly advises against this practice: "Don't use abstract classes to set behavior for classes". Prefer splitting behavior into interfaces (e.g., one for area calculation and one for drawing) and have figure classes implement those interfaces instead of relying on an abstract class for behavior. See the project checklist for the exact guidance: "Don't use abstract classes to set behavior for classes" . Also note the project's Figure base class is present in the codebase (Figure.java) which shows the current design choice .

@@ -0,0 +1,23 @@
package core.basesyntax;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are redundant empty lines at several places in this file (for example line 2, line 5, line 10, line 15, line 22). The checklist requires removing such empty lines: "Don't begin class or method implementation with an empty line. Remove all redundant empty lines, be careful :)" Please remove unnecessary blank lines to comply with the style rule .

- Added Figure interface with getArea() and draw() methods
- Renamed abstract class Figure to AbstractFigure
- Made AbstractFigure implement Figure
- Updated all shape classes to extend AbstractFigure
- Added AreaMeasurable (getArea) and Drawable (draw) interfaces
- Updated AbstractFigure to implement both
- Adjusted FigureSupplier and Main to use AbstractFigure hierarchy
Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a 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 checklist violations and one output formatting error that can break expected behavior/output.

Critical issues to fix:

  • Circle.draw(): change the area units from "aq. units" to "sq. units" to match the required output format. This can cause format-based checks to fail.
  • FigureSupplier and HelloWorld: replace magic literals with constants (e.g., DEFAULT_COLOR/DEFAULT_RADIUS for the default circle, and FIGURES_ARRAY_SIZE in main). This is an explicit checklist requirement.
  • FigureSupplier.getRandomFigure(): do not pass expressions directly into constructors. Assign random values to local variables first (e.g., int side = random.nextInt(MAX_SIZE) + 1; then new Square(color, side)).
  • ColorSupplier: reuse a class-level Random (private final Random random = new Random();) instead of creating a new Random inside getRandomColor().
  • Remove non-English (Ukrainian) comments in FigureSupplier and HelloWorld, or replace them with English, per the checklist.
  • Remove redundant leading/inner/trailing empty lines across files (AbstractFigure, AreaMeasurable, Drawable, Circle, Square, Rectangle, RightTriangle, IsoscelesTrapezoid, ColorSupplier, FigureSupplier, HelloWorld). The checklist explicitly forbids starting classes/methods with blank lines.

Minor note: Overall structure and area calculations look good, and the separation via interfaces is solid. After applying the fixes above, your solution should meet all requirements. Keep up the good work!


✨ 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

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

@@ -0,0 +1,13 @@
package core.basesyntax;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is an 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 this blank line so the class declaration follows the package statement directly.


public abstract class AbstractFigure implements AreaMeasurable, Drawable {
protected String color;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is an empty line inside the class 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 blank line to keep class internals compact.

public AbstractFigure(String color) {
this.color = color;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is an empty line inside the class after 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 blank line to keep class internals compact.

@@ -0,0 +1,6 @@
package core.basesyntax;

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 blank line between the package declaration and the interface declaration so the file starts immediately after the package line.

public interface AreaMeasurable {
double getArea();
}

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/trailing blank line at the end of the file to avoid extra empty lines.

public double getArea() {
return (firstLeg * secondLeg) / 2;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant empty line before the draw() override. The checklist requires: "Don't begin class or method implementation with an empty line. Remove all redundant empty lines, be careful :)" Remove this blank line so the @OverRide and method signature are contiguous with previous declarations .

@@ -0,0 +1,22 @@
package core.basesyntax;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a redundant empty line. It violates the checklist: "Don't begin class or method implementation with an empty line. Remove all redundant empty lines, be careful :)" Remove the extra blank line so the class declaration starts directly after the package declaration.


public class Square extends AbstractFigure {
private final double side;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This empty line before the constructor is redundant. The checklist requires: "Don't begin class or method implementation with an empty line. Remove all redundant empty lines, be careful :)" Remove this blank line so members begin without an extra empty line.

super(color);
this.side = side;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This empty line before the @OverRide of getArea() is redundant. The checklist requires: "Don't begin class or method implementation with an empty line. Remove all redundant empty lines, be careful :)" Remove it to comply with the style rule.

public double getArea() {
return side * side;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This empty line before the @OverRide of draw() is redundant. The checklist requires: "Don't begin class or method implementation with an empty line. Remove all redundant empty lines, be careful :)" Remove it so methods start immediately after previous members.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a 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 a few critical items that can break requirements and likely automated checks.

Critical issues that must be fixed:

  • Circle.draw output typo: change "aq. units" to "sq. units" to match the required output format. This can cause output-based tests to fail.
  • Public API/type mismatch: FigureSupplier methods return AbstractFigure, and HelloWorld uses AbstractFigure[]. The task expects a top-level abstraction (e.g., Figure) and specifically a public Figure getRandomFigure() method. Please return the expected abstraction and use it in main to align with the specification and avoid signature-based test failures.
  • ColorSupplier should reuse a class-level Random instance instead of creating a new one in the method, per the checklist.
  • FigureSupplier passes expressions directly into constructors. Create local variables for each random dimension (e.g., int side = ...;) and pass them to constructors, as required by the checklist.
  • Magic literals: replace the hard-coded 6 in HelloWorld with a named constant, and extract default values in FigureSupplier (e.g., DEFAULT_COLOR, DEFAULT_RADIUS) instead of using string/number literals.
  • Remove redundant empty lines at the start of files and before method/class implementations across files (AreaMeasurable, Drawable, ColorSupplier, Circle, Rectangle, RightTriangle, IsoscelesTrapezoid, HelloWorld). This is explicitly required by the checklist.

Minor (still fix for compliance): replace Ukrainian comments with English in FigureSupplier and HelloWorld.

Positives: your figure classes compute areas correctly and print the required details; shared state and behavior separation via AbstractFigure, AreaMeasurable, and Drawable is solid; FigureSupplier uses class-level Random and constants for bounds. After addressing the items above, your solution should fully meet the task requirements. Keep it up!


✨ 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

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

@@ -0,0 +1,6 @@
package core.basesyntax;

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 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 :)" Please remove the blank line so the interface starts directly after the package statement.

public interface AreaMeasurable {
double getArea();
}

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 near the end of the file before the final non-commentable marker. Remove this redundant empty line to comply with the checklist instruction: "Don't begin class or method implementation with an empty line. Remove all redundant empty lines, be careful :)"

@@ -0,0 +1,22 @@
package core.basesyntax;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is an empty line between the package declaration and the class declaration. It 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 immediately follows the package statement.


public class Circle extends AbstractFigure {
private final double radius;

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 right before the constructor. The checklist requires: "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 starts directly after the field declaration.

super(color);
this.radius = radius;
}

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 immediately before the getArea() method. Per the checklist: "Don't begin class or method implementation with an empty line. Remove all redundant empty lines, be careful :)" Remove the blank line so method implementations do not start with an empty line.

public double getArea() {
return (firstLeg * secondLeg) / 2;
}

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 immediately 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 this blank line so the draw() method starts without a preceding empty line.

@@ -0,0 +1,22 @@
package core.basesyntax;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is an empty line immediately after the package declaration. This violates the checklist: "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 follows the package statement directly.


public class Square extends AbstractFigure {
private final double side;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is an empty line before the constructor. This violates the checklist: "Don't begin class or method implementation with an empty line. Remove all redundant empty lines, be careful :)" Remove the blank line so the constructor starts directly after the field declaration.

super(color);
this.side = side;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is an empty line before the getArea() method. This violates the checklist: "Don't begin class or method implementation with an empty line. Remove all redundant empty lines, be careful :)" Remove the blank line so method implementations do not start with an empty line.

public double getArea() {
return side * side;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is an empty line before the draw() method. This violates the checklist: "Don't begin class or method implementation with an empty line. Remove all redundant empty lines, be careful :)" Remove the blank line so the draw() method starts without a preceding empty line.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a 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.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a 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.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a 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.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a 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.

Copy link

@Elena-Bruyako Elena-Bruyako left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comments

@@ -0,0 +1,6 @@
package core.basesyntax;

public interface AreaMeasurable {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public interface AreaMeasurable {
public interface AreaCalculator {

private final Random random = new Random();
private final ColorSupplier colorSupplier = new ColorSupplier();

// Змінено тип повернення на AbstractFigure

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove comment

};
}

// і тут теж

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove comment


// і тут теж
public AbstractFigure getDefaultFigure() {
return new Circle("white", 10);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

10 is magic number, create constant

package core.basesyntax;

public class IsoscelesTrapezoid extends AbstractFigure {
private final double base1;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't use numbers for naming

String color = colorSupplier.getRandomColor();

return switch (figureType) {
case 0 -> new Square(color, random.nextInt(MAX_SIZE) + 1);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

create separate method for getting random side random.nextInt(MAX_SIZE) + 1


// і тут теж
public AbstractFigure getDefaultFigure() {
return new Circle("white", 10);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use WHITE from enum

public class HelloWorld {
public static void main(String[] args) {
FigureSupplier figureSupplier = new FigureSupplier();
AbstractFigure[] figures = new AbstractFigure[6]; // тут AbstractFigure

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

6 is a magic number, create constant

FigureSupplier figureSupplier = new FigureSupplier();
AbstractFigure[] figures = new AbstractFigure[6]; // тут AbstractFigure

for (int i = 0; i < figures.length / 2; i++) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use one for loop to implement main method

}

for (AbstractFigure figure : figures) {
figure.draw(); // draw() є в AbstractFigure через Drawable

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants