done #1779

xMaXD · 2025-01-08T18:47:30Z

No description provided.

mateacademy-ai-mentor

Hello! 😊 You've done a great job implementing the classes and methods, and your code mostly meets the task requirements. However, there are a couple of critical issues that need to be addressed before we can approve the pull request:

Returning null from a method: In the FigureSupplier class, the getRandomFigure() method returns null in the default case of the switch statement. This could lead to NullPointerException and is against best practices. Consider returning a default figure or handling this case differently.
Magic numbers: The use of magic numbers, such as 5 in the random.nextInt(5) call, should be replaced with constants. This will improve the readability and maintainability of your code.

Once these issues are resolved, your code will be in great shape! Keep up the good work, and I'm looking forward to seeing the improvements. 🚀

✨ 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.

mateacademy-ai-mentor · 2025-01-08T18:55:24Z

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

+        ColorSupplier colorSupplier = new ColorSupplier();
+        Random random = new Random();
+        String color = colorSupplier.getRandomColor();
+        int figureType = random.nextInt(5);


Consider defining a constant for the number of figure types (5) to avoid magic numbers and improve code readability.

mateacademy-ai-mentor · 2025-01-08T18:55:24Z

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

+                    random.nextDouble() * 10,
+                    random.nextDouble() * 10,
+                    random.nextDouble() * 10);
+            default: return null;


Returning null from a method is a bad practice. Instead of returning null in the default case, you could return a default figure or handle this case differently to avoid potential NullPointerException.

mateacademy-ai-mentor

Review Summary

Great job on implementing the various classes and interfaces! Your code is well-structured and demonstrates a solid understanding of object-oriented principles. Here are some key points from the review:

Magic Numbers: You've successfully replaced the magic number 5 with a constant FIGURE_COUNT in the FigureSupplier class, which enhances readability and maintainability. This is a good practice to follow in future projects as well.
Null Handling: The getRandomFigure() method in the FigureSupplier class no longer returns null, which is a significant improvement. Instead, it now returns a default figure using the getDefaultFigure() method, ensuring that your code is more robust and less prone to errors.
Class Implementations: All classes, such as Circle, Rectangle, Square, RightTriangle, and IsoscelesTrapezoid, are well-implemented. They correctly extend the Figure class and override necessary methods like getArea and draw. Your constructors are also well-defined, initializing attributes appropriately.
Interfaces and Abstract Classes: The Shape and Colored interfaces are correctly defined, and the Figure abstract class implements them properly. This setup allows for flexibility and scalability in your code.
Main Class: The Main class is effectively set up to demonstrate the functionality of your FigureSupplier class, showcasing your understanding of how to integrate and test your components.

Overall, your code meets the task requirements and constraints. While there are no critical issues, always strive to review and refine your code based on feedback. This will help you improve and learn from any mistakes. Keep up the great work, and continue to build on your skills! 🚀

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

mateacademy-ai-mentor · 2025-01-09T16:20:55Z

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

+        ColorSupplier colorSupplier = new ColorSupplier();
+        Random random = new Random();
+        String color = colorSupplier.getRandomColor();
+        final int figureType = random.nextInt(5);


Consider replacing the magic number 5 with a named constant to improve readability and maintainability. For example, you could define a constant private static final int NUMBER_OF_FIGURE_TYPES = 5; and use it here.

liliia-ponomarenko

Good job! Let’s improve your solution ;)

liliia-ponomarenko · 2025-01-10T07:14:46Z

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

+
+

Suggested change

Leave only one empty line at the end of the file (check it everywhere)

liliia-ponomarenko · 2025-01-10T07:15:44Z

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"};


Let’s create separate enum class Color

liliia-ponomarenko · 2025-01-10T07:16:20Z

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

+    private static final String[] COLORS = {"Red", "Green", "Blue", "Yellow", "Black", "White"};
+
+    public String getRandomColor() {
+        Random random = new Random();


Make Random private final class level variable

liliia-ponomarenko · 2025-01-10T07:17:39Z

src/main/java/core/basesyntax/Colored.java

+public interface Colored {
+    String getColor();
+}
+


remove this interface Color is state (not behavior)

liliia-ponomarenko · 2025-01-10T07:18:04Z

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

+public abstract class Figure implements Shape, Colored {
+    private String color;
+


Suggested change

public abstract class Figure implements Shape, Colored {

private String color;

public abstract class Figure {

private Color color;

liliia-ponomarenko · 2025-01-10T07:18:57Z

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

+        ColorSupplier colorSupplier = new ColorSupplier();
+        Random random = new Random();


should be class level variable

liliia-ponomarenko · 2025-01-10T07:19:12Z

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

+        ColorSupplier colorSupplier = new ColorSupplier();
+        Random random = new Random();
+        String color = colorSupplier.getRandomColor();
+        final int figureType = random.nextInt(5);


use constant

liliia-ponomarenko · 2025-01-10T07:19:57Z

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

+
+        switch (figureType) {
+            case 0: return new Square(color,
+                    random.nextDouble() * 10);


the same. make 10 constant and reuse it

liliia-ponomarenko · 2025-01-10T07:20:10Z

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

+    }
+
+    public Figure getDefaultFigure() {
+        return new Circle("White", 10);


use constants

liliia-ponomarenko · 2025-01-10T07:21:11Z

src/main/java/core/basesyntax/Shape.java

+    double getArea();
+
+    void draw();


create interfaces AreaCalculator with method getArea() and Drawable with draw()

done

9fa2b55

mateacademy-ai-mentor suggested changes Jan 8, 2025

View reviewed changes

fix

c3b5a8d

xMaXD requested a review from mateacademy-ai-mentor January 9, 2025 16:16

mateacademy-ai-mentor approved these changes Jan 9, 2025

View reviewed changes

liliia-ponomarenko suggested changes Jan 10, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

done #1779

done #1779

xMaXD commented Jan 8, 2025

mateacademy-ai-mentor left a comment

mateacademy-ai-mentor Jan 8, 2025

mateacademy-ai-mentor Jan 8, 2025

mateacademy-ai-mentor left a comment

mateacademy-ai-mentor Jan 9, 2025

liliia-ponomarenko left a comment

liliia-ponomarenko Jan 10, 2025

liliia-ponomarenko Jan 10, 2025

liliia-ponomarenko Jan 10, 2025

liliia-ponomarenko Jan 10, 2025

liliia-ponomarenko Jan 10, 2025

liliia-ponomarenko Jan 10, 2025

liliia-ponomarenko Jan 10, 2025

liliia-ponomarenko Jan 10, 2025

liliia-ponomarenko Jan 10, 2025

liliia-ponomarenko Jan 10, 2025

		public abstract class Figure implements Shape, Colored {
		private String color;

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

done #1779

Are you sure you want to change the base?

done #1779

Conversation

xMaXD commented Jan 8, 2025

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

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Choose a reason for hiding this comment

liliia-ponomarenko 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