hw-1-jv-oop-advanced #1741

Oleksandr19891989 · 2024-11-11T20:00:00Z

No description provided.

mateacademy-ai-mentor

Thank you for your hard work on this project! While your implementation is mostly correct and meets many of the task requirements, there are a few critical issues that need to be addressed before we can approve the pull request:

Magic Numbers: The use of magic numbers, such as 5 in random.nextInt(5), should be replaced with constants. This will enhance the readability and maintainability of your code.
Code Readability: For better readability, it's recommended to create separate variables for random values before passing them to constructors. This small change can make your code much easier to understand.
Constructor Formatting: There are minor formatting issues in the constructor parameter lists, such as missing or extra spaces around commas. These should be corrected for consistency and readability.
super.draw() Calls: Ensure that the super.draw() calls are necessary. If the Figure class does not have a draw() method or if it does not need to be called, these lines should be removed.
Interface Definition: Make sure that the Measurable interface is defined and includes the getArea() method, as its absence will lead to compilation errors.

Addressing these issues will not only improve the quality of your code but also ensure it functions correctly and efficiently. Keep up the great work, and I'm confident you'll nail it with these adjustments! 😊

✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨

🔄 Fix the issues, commit the changes, and then re-request my review using the Re-request review button in the Reviewers tab.

src/main/java/core/basesyntax/Circle.java

mateacademy-ai-mentor · 2024-11-11T20:04:12Z

src/main/java/core/basesyntax/Circle.java

+
+    @Override
+    public void draw() {
+        super.draw();


Ensure that the super.draw() call is necessary. This depends on the implementation of the Figure class. If Figure does not have a draw() method or if it does not need to be called, this line can be removed.

mateacademy-ai-mentor · 2024-11-11T20:04:12Z

src/main/java/core/basesyntax/Figure.java

@@ -0,0 +1,13 @@
+package core.basesyntax;
+
+public abstract class Figure implements Drawable,Measurable{


Ensure that the Measurable interface is defined and includes the getArea() method, as it is used in the draw() method. If Measurable is not defined, this will cause a compilation error.

mateacademy-ai-mentor · 2024-11-11T20:04:12Z

src/main/java/core/basesyntax/FigureSupplier.java

+
+    public Figure getRandomFigure() {
+        String color = colorSupplier.getRandomColor();
+        switch (random.nextInt(5)) {


Consider defining a constant for the number of figure types instead of using the magic number 5. This will improve code readability and maintainability. For example, private static final int FIGURE_TYPES_COUNT = 5; and use it in random.nextInt(FIGURE_TYPES_COUNT).

mateacademy-ai-mentor · 2024-11-11T20:04:13Z

src/main/java/core/basesyntax/FigureSupplier.java

+                return new Square(color, random.nextInt(10) + 1);
+            case 1:
+                return new Rectangle(color, random.nextInt(10) + 1, random.nextInt(10) + 1);
+            case 2:
+                return new RightTriangle(color, random.nextInt(10) + 1, random.nextInt(10) + 1);
+            case 3:
+                return new Circle(color, random.nextInt(10) + 1);
+            default:
+                return new IsoscelesTrapezoid(color, random.nextInt(10) + 1, random.nextInt(10) + 1, random.nextInt(10) + 1);


It's recommended to create separate variables for random values before passing them to constructors. This improves code readability. For example, int side = random.nextInt(10) + 1; and then use new Square(color, side);.

mateacademy-ai-mentor · 2024-11-11T20:04:13Z

src/main/java/core/basesyntax/RightTriangle.java

+
+    @Override
+    public void draw() {
+        super.draw();


Ensure that the super.draw() call is necessary. This depends on the implementation of the Figure class. If Figure does not have a draw() method or if it does not need to be called, this line can be removed.

mateacademy-ai-mentor · 2024-11-11T20:04:13Z

src/main/java/core/basesyntax/Square.java

+public class Square extends Figure {
+    private double side;
+
+    public Square(String color,double side) {


There is a missing space after the comma in the constructor parameter list String color,double side. It should be String color, double side.

mateacademy-ai-mentor · 2024-11-11T20:04:14Z

src/main/java/core/basesyntax/Square.java

+
+    public Square(String color,double side) {
+        super(color);
+        this.side=side;


There should be spaces around the assignment operator for better readability: this.side = side;.

mateacademy-ai-mentor · 2024-11-11T20:04:14Z

src/main/java/core/basesyntax/Square.java

+
+    @Override
+    public void draw() {
+        super.draw();


Ensure that the super.draw() call is necessary. This depends on the implementation of the Figure class. If Figure does not have a draw() method or if it does not need to be called, this line can be removed.

mateacademy-ai-mentor · 2024-11-11T20:04:14Z

src/main/java/core/basesyntax/Square.java

+
+    @Override
+    public double getArea() {
+        return side*side;


There should be spaces around the multiplication operator for better readability: return side * side;.

…lasses

okuzan

Please make sure build passes locally (with mvn clean package) before requesting mentors' review

ahoienko

Nice, left some comments

ahoienko · 2024-11-15T15:16:48Z

src/main/java/core/basesyntax/ColorSupplier.java

+import java.util.Random;
+
+public class ColorSupplier {
+    private static final String[] COLORS = {"red", "green", "blue", "yellow", "black", "white"};


Better to create Color enum, and use it in all figures as field. It would be easier and more clear to extend code. And also you would avoid issues when someone made a mistake writing some color

ahoienko · 2024-11-15T15:17:19Z

src/main/java/core/basesyntax/FigureSupplier.java

+                int side = random.nextInt(10) + 1;
+                return new Square(color, side);
+            }
+            case 1: {
+                int width = random.nextInt(10) + 1;
+                int height = random.nextInt(10) + 1;
+                return new Rectangle(color, width, height);
+            }
+            case 2: {
+                int base = random.nextInt(10) + 1;
+                int height = random.nextInt(10) + 1;
+                return new RightTriangle(color, base, height);
+            }
+            case 3: {
+                int radius = random.nextInt(10) + 1;
+                return new Circle(color, radius);
+            }
+            default: {
+                int base1 = random.nextInt(10) + 1;
+                int base2 = random.nextInt(10) + 1;
+                int height = random.nextInt(10) + 1;


magic numbers

ahoienko · 2024-11-15T15:17:46Z

src/main/java/core/basesyntax/Main.java

+public class Main {
+    public static void main(String[] args) {
+        FigureSupplier figureSupplier = new FigureSupplier();
+        Figure[] figures = new Figure[6];


Magic number

ahoienko · 2024-11-15T15:18:23Z

src/main/java/core/basesyntax/Main.java

+        for (int i = 0; i < figures.length / 2; i++) {
+            figures[i] = figureSupplier.getRandomFigure();
+        }
+
+        // Generate default figures for the second half
+        for (int i = figures.length / 2; i < figures.length; i++) {
+            figures[i] = figureSupplier.getDefaultFigure();
+        }
+
+        // Display all figures
+        for (Figure figure : figures) {
+            figure.draw();
+            System.out.println();
+        }


It is possible to make everything in one loop

ahoienko · 2024-11-15T15:20:50Z

src/main/java/core/basesyntax/Measurable.java

@@ -0,0 +1,5 @@
+package core.basesyntax;
+
+public interface Measurable {


Suggested change

public interface Measurable {

public interface AreaCalcualtor{

ahoienko · 2024-11-15T15:21:03Z

src/main/java/core/basesyntax/Square.java

+
+    @Override
+    public void draw() {
+        super.draw();


okuzan · 2024-11-17T11:33:40Z

src/main/java/core/basesyntax/ColorSupplier.java

+public class ColorSupplier {
+    private static final Random RANDOM = new Random();
+
+    public Color getRandomColor() {


in task description, this method should return String

okuzan · 2024-11-17T11:34:07Z

src/main/java/core/basesyntax/IsoscelesTrapezoid.java

+    private double height;
+
+    public IsoscelesTrapezoid(Color color, double base1, double base2, double height) {
+        super(String.valueOf(color));


we should pass String to constructor.
Also, to get string representation of enum use .name()

okuzan · 2024-11-17T11:34:23Z

src/main/java/core/basesyntax/IsoscelesTrapezoid.java

+    private double base1;
+    private double base2;


numbers in naming are usually bad, let's name them topBase bottomBase

Elena-Bruyako · 2024-11-19T12:31:51Z

src/main/java/core/basesyntax/Color.java

+package core.basesyntax;
+
+public enum Color {
+    RED, GREEN, BLUE, YELLOW, BLACK, WHITE;


Suggested change

RED, GREEN, BLUE, YELLOW, BLACK, WHITE;

RED,

GREEN,

BLUE,

YELLOW,

BLACK,

WHITE

Elena-Bruyako · 2024-11-19T12:32:56Z

src/main/java/core/basesyntax/ColorSupplier.java

+
+    public String getRandomColor() {
+        Color[] colors = Color.values();
+        return colors[RANDOM.nextInt(colors.length)].toString();


Use name() for getting String representation of enum constants

Elena-Bruyako · 2024-11-19T12:33:55Z

src/main/java/core/basesyntax/FigureSupplier.java

+        String color = colorSupplier.getRandomColor();
+        switch (random.nextInt(FIGURE_TYPES_COUNT)) {
+            case 0: {
+                int side = random.nextInt(MAX_DIMENSION) + MIN_DIMENSION;


let's avoid duplication of code
create separate method to get random side

Elena-Bruyako · 2024-11-19T12:34:45Z

src/main/java/core/basesyntax/Circle.java

+public class Circle extends Figure {
+    private double radius;
+
+    public Circle(Color color, double radius) {


Suggested change

public Circle(Color color, double radius) {

public Circle(String color, double radius) {

For each figure

Elena-Bruyako · 2024-11-19T12:34:53Z

src/main/java/core/basesyntax/Circle.java

+    private double radius;
+
+    public Circle(Color color, double radius) {
+        super(color.name());


Suggested change

super(color.name());

super(color);

Elena-Bruyako · 2024-11-19T12:36:04Z

src/main/java/core/basesyntax/Main.java

+            }
+        }
+
+        for (Figure figure : figures) {


use one for loop to implement main method

Elena-Bruyako · 2024-11-19T12:37:43Z

src/main/java/core/basesyntax/Main.java

+        final int arraySize = (int) (Math.random() * 10) + 1;
+        Figure[] figures = new Figure[arraySize];


create constant SIZE_OF_ARRAY for example 6

Suggested change

final int arraySize = (int) (Math.random() * 10) + 1;

Figure[] figures = new Figure[arraySize];

Figure[] figures = new Figure[SIZE_OF_ARRAY];

Elena-Bruyako

fix all previous comments

Elena-Bruyako · 2024-11-23T11:08:16Z

src/main/java/core/basesyntax/Circle.java

+public class Circle extends Figure {
+    private double radius;
+
+    public Circle(Color color, double radius) {


Elena-Bruyako · 2024-11-23T11:08:21Z

src/main/java/core/basesyntax/Circle.java

+    private double radius;
+
+    public Circle(Color color, double radius) {
+        super(color.name());


Elena-Bruyako · 2024-11-23T11:08:25Z

src/main/java/core/basesyntax/Color.java

+package core.basesyntax;
+
+public enum Color {
+    RED, GREEN, BLUE, YELLOW, BLACK, WHITE;


Elena-Bruyako · 2024-11-23T11:08:30Z

src/main/java/core/basesyntax/ColorSupplier.java

+
+    public String getRandomColor() {
+        Color[] colors = Color.values();
+        return colors[RANDOM.nextInt(colors.length)].toString();


Elena-Bruyako · 2024-11-23T11:08:53Z

src/main/java/core/basesyntax/FigureSupplier.java

+        String color = colorSupplier.getRandomColor();
+        switch (random.nextInt(FIGURE_TYPES_COUNT)) {
+            case 0: {
+                int side = random.nextInt(MAX_DIMENSION) + MIN_DIMENSION;


Elena-Bruyako · 2024-11-23T11:09:00Z

src/main/java/core/basesyntax/Main.java

+        final int arraySize = (int) (Math.random() * 10) + 1;
+        Figure[] figures = new Figure[arraySize];


Elena-Bruyako · 2024-11-23T11:09:06Z

src/main/java/core/basesyntax/Main.java

+            }
+        }
+
+        for (Figure figure : figures) {


hw-1-jv-oop-advanced

640268c

mateacademy-ai-mentor suggested changes Nov 11, 2024

View reviewed changes

OleksandrDarchyk added 6 commits November 11, 2024 21:36

hw-1-jv-oop-advanced

fb52199

hw-1-jv-oop-advanced

77f6840

hw-2-oop-advanced

21d4614

hw-1-jv-oop-advanced - updated Rectangle, RightTriangle, and Square c…

50ed5af

…lasses

hw-1-jv-oop-advanced - updated Rectangle, RightTriangle, and Square c…

7869b7d

…lasses

hw-1-jv-oop-advanced - updated Rectangle, RightTriangle, and Square c…

696a001

…lasses

Oleksandr19891989 closed this Nov 11, 2024

Oleksandr19891989 reopened this Nov 11, 2024

OleksandrDarchyk added 2 commits November 11, 2024 22:31

hw-1-jv-oop-advanced - updated Rectangle, RightTriangle, and Square c…

c9b1896

…lasses

Оновив Rectangle, RightTriangle та Square

880f82b

Oleksandr19891989 requested a review from mateacademy-ai-mentor November 11, 2024 21:45

okuzan suggested changes Nov 12, 2024

View reviewed changes

mvn clean package

f40894a

Oleksandr19891989 requested a review from okuzan November 15, 2024 12:19

ahoienko suggested changes Nov 15, 2024

View reviewed changes

chenges

7b63e17

Oleksandr19891989 requested a review from ahoienko November 15, 2024 20:06

okuzan suggested changes Nov 17, 2024

View reviewed changes

chenges

c09aec3

Oleksandr19891989 requested a review from okuzan November 19, 2024 11:26

Elena-Bruyako suggested changes Nov 19, 2024

View reviewed changes

Oleksandr19891989 requested a review from Elena-Bruyako November 22, 2024 17:26

Elena-Bruyako suggested changes Nov 23, 2024

View reviewed changes

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

		public abstract class Figure implements Drawable,Measurable{

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

		public interface Measurable {

	public interface Measurable {
	public interface AreaCalcualtor{

	public Circle(Color color, double radius) {
	public Circle(String color, double radius) {

		final int arraySize = (int) (Math.random() * 10) + 1;
		Figure[] figures = new Figure[arraySize];

	final int arraySize = (int) (Math.random() * 10) + 1;
	Figure[] figures = new Figure[arraySize];
	Figure[] figures = new Figure[SIZE_OF_ARRAY];

hw-1-jv-oop-advanced #1741

Are you sure you want to change the base?

hw-1-jv-oop-advanced #1741

Conversation

Oleksandr19891989 commented Nov 11, 2024

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

okuzan left a comment

Choose a reason for hiding this comment

ahoienko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Elena-Bruyako left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment