-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Hw oop advanced #1714
base: master
Are you sure you want to change the base?
Hw oop advanced #1714
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.
Great job on your code! You've adhered to the task requirements and checklist well. There are no significant issues in your code. Keep up the good work! 🌟
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
@@ -0,0 +1,19 @@ | |||
package core.basesyntax; | |||
|
|||
public abstract class Figure implements Draw { |
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.
According to the checklist, we should avoid using abstract classes to set behavior for classes. Instead, you should use interfaces. In this case, you could create an interface named 'Figure' with the method 'getArea()' and another interface named 'Colored' with the methods 'getColor()' and 'setColor(String color)'. Then, each figure class could implement these interfaces as needed.
this.color = color; | ||
} | ||
|
||
public abstract double getArea(); |
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 method 'getArea()' should be in a separate interface. The 'Figure' class should only deal with properties that are common to all figures, like color. The calculation of the area is specific to each type of figure, so it should be in a separate interface.
int side = random.nextInt(MAXIMUM); | ||
return new Square(color, side); | ||
case 1: | ||
int firstSide = random.nextInt(MAXIMUM); | ||
int secondSide = random.nextInt(MAXIMUM); | ||
return new Rectangle(color, firstSide, secondSide); | ||
case 2: | ||
int firstLeg = random.nextInt(MAXIMUM); | ||
int secondLeg = random.nextInt(MAXIMUM); | ||
return new RightTriangle(color, firstLeg, secondLeg); | ||
case 3: | ||
int radius = random.nextInt(MAXIMUM); | ||
return new Circle(color, radius); | ||
case 4: | ||
int firstBase = random.nextInt(MAXIMUM); | ||
int secondBase = random.nextInt(MAXIMUM); | ||
int height = random.nextInt(MAXIMUM); |
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.
Creating a figure, don't pass expressions in the constructor. Create separate variables and pass them on for better code readability. It will make your code cleaner and easier to debug.
} | ||
|
||
public Figure getDefaultFigure() { | ||
return new Circle(Color.WHITE.name(), 10); |
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.
It's better to create a constant for the default figure size. It's a good practice to extract magic numbers into constants. It makes your code easier to read and maintain.
System.out.println("The Figure: Trapezoid; First base = " + firstBase | ||
+ " Second base = " + secondBase + " Height = " + height | ||
+ "\n" + " Area: " + getArea() | ||
+ "; Color: " + getColor() + "\n"); |
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.
You should avoid using string concatenation in System.out.println()
statement. It's better to use String.format()
method for such purposes. This way you can create a template for your message and then just pass the required variables as arguments. It will also help to avoid potential issues with '+' operator precedence.
public double getArea() { | ||
return 0.5 * (firstBase + secondBase) * height; | ||
} |
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.
According to the checklist, all magic numbers in your code should be constants. Here, 0.5
is a magic number. Please, create a constant for it.
|
||
public interface Area { | ||
double getArea(); | ||
|
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.
Remove all redundant empty lines, be careful :)
@@ -0,0 +1,6 @@ | |||
package core.basesyntax; | |||
|
|||
public interface Area { |
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.
- Don't use verbs for class/interface names
https://github.com/mate-academy/jv-oop-advanced/blob/master/checklist.md#dont-use-verbs-for-classinterface-names - Seem like you don't use this interface anywhere, so instead of abstract method in class Figure just implement this interface
package core.basesyntax; | ||
|
||
public interface Area { | ||
double getArea(); |
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.
Better name will be calculateArea()
@@ -0,0 +1,5 @@ | |||
package core.basesyntax; | |||
|
|||
public interface Draw { |
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.
Don't use verbs for class/interface names
https://github.com/mate-academy/jv-oop-advanced/blob/master/checklist.md#dont-use-verbs-for-classinterface-names
package core.basesyntax; | ||
|
||
public interface Draw { | ||
void toDraw(); |
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.
Better name will be draw()
System.out.println("The Figure: Circle; radius = " + radius | ||
+ "\n" + " Area: " + getArea() | ||
+ "; Color: " + getColor() + "\n"); | ||
} |
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.
Please, pay attention to the example result in task description. For all figures
private static final Random random = new Random(); | ||
|
||
public String getRandomColor() { | ||
int index = random.nextInt(Color.values().length); | ||
Color color = Color.values()[index]; | ||
return color.name(); | ||
} | ||
|
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.
You can create a constant variable to save number of colors and you can use this constant in method random.nextInt()
} | ||
|
||
public Figure getDefaultFigure() { | ||
return new Circle(Color.WHITE.name(), 10); |
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.
All magic numbers in your code should be constants
https://github.com/mate-academy/jv-oop-advanced/blob/master/checklist.md#all-magic-numbers-in-your-code-should-be-constants
*/ | ||
public class Main { | ||
public static void main(String[] args) { | ||
Figure[] figures = new Figure[6]; |
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.
All magic numbers in your code should be constants
https://github.com/mate-academy/jv-oop-advanced/blob/master/checklist.md#all-magic-numbers-in-your-code-should-be-constants
for (int i = 0; i < figures.length; i++) { | ||
figures[i] = (i <= figures.length / 2 - 1) | ||
? figureSupplier.getRandomFigure() | ||
: figureSupplier.getDefaultFigure(); | ||
} |
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.
You can calculate the middle of figures array before loop and just use it value in if statement
No description provided.