Skip to content
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

Solution jv-oop-advanced #1721

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

SnkvS
Copy link

@SnkvS SnkvS commented Oct 18, 2024

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.

Thank you for your hard work on this pull request! There are a few critical issues that need to be addressed, such as the incorrect class name in the toString() method and the use of an abstract class to set behavior. Once these are resolved, your code will be in great shape. Keep up the great work! 😊

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

return radius;
}

public void setRadius(int radius) {

Choose a reason for hiding this comment

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

The parameter type for the setRadius method should be consistent with the type of the radius field, which is double. Consider changing the parameter type from int to double.

Comment on lines 8 to 9
public String getRandomColor() {
return Colors.values()[random.nextInt(Colors.values().length)].toString();

Choose a reason for hiding this comment

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

Instead of using toString() to get the String representation of an enum constant, use the name() method. This ensures consistency and avoids issues if toString() is overridden.

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

public abstract class Figure implements AreaCalculator, Drawable {

Choose a reason for hiding this comment

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

The class Figure is using an abstract class to set behavior by implementing both AreaCalculator and Drawable interfaces. According to the checklist, abstract classes should not be used to set behavior. Consider using interfaces to define behaviors instead.


public abstract class Figure implements AreaCalculator, Drawable {
private String color;
private double area;

Choose a reason for hiding this comment

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

The area field is declared but not used in this class. If it's intended to store the area of the figure, consider calculating and setting it within the class methods or remove it if it's unnecessary.


import java.util.Random;

public class FigureSupplierImpl implements FigureSupplier {

Choose a reason for hiding this comment

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

The class name FigureSupplierImpl uses a verb, which is not recommended. Consider renaming it to something like FigureSupplierImplementation or another noun-based name.

@Override
public Figure getDefaultFigure() {
var c = new Circle(10.0);
c.setColor(Colors.WHITE.toString());

Choose a reason for hiding this comment

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

The checklist advises using name() instead of toString() for getting the string representation of enum constants. Replace Colors.WHITE.toString() with Colors.WHITE.name().

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

public class IsoscelesTrapezoid extends Figure {

Choose a reason for hiding this comment

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

Don't use verbs for class/interface names. Consider renaming IsoscelesTrapezoid to a noun form if possible, although in this case, it might be acceptable as it is a specific shape name.


public class Main {
public static void main(String[] args) {
var supplierFigure = new FigureSupplierImpl();

Choose a reason for hiding this comment

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

It's recommended not to use 'var' for variable declarations in this context. Instead, specify the type explicitly for better readability and understanding of the code.

public class Main {
public static void main(String[] args) {
var supplierFigure = new FigureSupplierImpl();
var supplierColor = new ColorSupplier();

Choose a reason for hiding this comment

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

It's recommended not to use 'var' for variable declarations in this context. Instead, specify the type explicitly for better readability and understanding of the code.


@Override
public String toString() {
final StringBuilder sb = new StringBuilder("Rectangle{");

Choose a reason for hiding this comment

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

There is a mistake in the toString() method. The class name should be RightTriangle instead of Rectangle.

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.

Great job on implementing the classes and interfaces! 🎉 While there are a few areas for improvement, such as ensuring array lengths are even and avoiding returning null from methods, your work aligns well with the task requirements. Remember to address these issues in future projects to enhance your code quality. Keep up the good work and continue learning from these experiences!

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

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

public interface AreaCalculator {

Choose a reason for hiding this comment

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

The interface name 'AreaCalculator' is a noun, which is good. However, make sure that this interface is used only for calculating areas, as per the checklist item: 'Don't put all behavior into a single interface if the methods are conceptually different from each other.'

@@ -0,0 +1,35 @@
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.

The class name 'Circle' is appropriate as it is a noun and represents an entity.

}

@Override
public double calculateArea() {

Choose a reason for hiding this comment

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

The method 'calculateArea' is correctly overridden to calculate the area of the circle.

}

@Override
public String toString() {

Choose a reason for hiding this comment

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

The 'toString' method provides a string representation of the Circle object, which is useful for debugging and logging.

}

@Override
public void draw() {

Choose a reason for hiding this comment

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

The 'draw' method is correctly overridden to print the Circle object, which aligns with the Drawable interface.


public class Main {
public static void main(String[] args) {
FigureSupplier supplierFigure = new FigureSupplierImplementation();

Choose a reason for hiding this comment

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

Avoid using verbs for class names. Consider renaming FigureSupplierImplementation to something more descriptive and noun-based, like FigureSupplierImpl.

FigureSupplier supplierFigure = new FigureSupplierImplementation();
ColorSupplier supplierColor = new ColorSupplier();
Figure[] array = new Figure[6];
for (int i = 0; i < array.length / 2; i++) {

Choose a reason for hiding this comment

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

The loop condition i < array.length / 2 is correct, but ensure that the array length is even to avoid potential issues. If the length is odd, this logic might not fill the array as intended.

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

Choose a reason for hiding this comment

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

Don't use verbs for class/interface names. Consider renaming Rectangle to a noun form if applicable.

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

public class RightTriangle extends Figure {

Choose a reason for hiding this comment

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

The class name 'RightTriangle' is appropriate as it represents a noun, which is a good practice for class names.

final StringBuilder sb = new StringBuilder("RightTriangle{");
sb.append("firstLeg=").append(firstLeg);
sb.append(", secondLeg=").append(secondLeg);
sb.append(", color").append(getColor());

Choose a reason for hiding this comment

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

There is a missing '=' sign in the toString() method for the 'color' field. It should be 'sb.append(", color=").append(getColor());'.

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