-
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
implemented all the methods from the task #1284
base: master
Are you sure you want to change the base?
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.
Good job!
Please, pay attention to my comments.
@@ -0,0 +1,23 @@ | |||
package core.basesyntax; | |||
|
|||
public class Figure implements Drawable, AreaCalculator { |
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.
I think we can make this class abstract.
@Override | ||
public void draw() { | ||
|
||
} | ||
|
||
@Override | ||
public double getArea() { | ||
return 0; | ||
} |
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.
If we make class abstract we will be able to remove that emty overriding.
public Circle() { | ||
|
||
} |
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.
I think we should reject creating empty Circle. Let's remove that default constructor.
package core.basesyntax; | ||
|
||
public class Figure implements Drawable, AreaCalculator { | ||
private Color color; |
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.
Add a constructor public Figure(String color)
|
||
public Circle(int radius, Color color) { | ||
this.radius = radius; | ||
this.setColor(color); |
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.
Replace it with super(color);
.
default: | ||
return new Figure(); | ||
} | ||
|
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.
Empty redundant line.
} | ||
|
||
public Figure getDefaultFigure() { | ||
return new Circle(10, Color.WHITE); |
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.
Looks like a magic number.
|
||
public class Main { | ||
public static void main(String[] args) { | ||
Drawable[] figures = new Drawable[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.
Looks like a magic number.
Color randomColor = colorSupplier.getRandomColor(); | ||
int figureIndex = random.nextInt(MAX_FIGURES_TYPES) + 1; | ||
int sideA = random.nextInt(SIZE_LIMIT) + 1; | ||
switch (figureIndex) { |
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 simplify it with switch (random.nextInt(MAX_FIGURES_TYPES) + 1)
.
int trapezoidSideB = random.nextInt(SIZE_LIMIT) + 1; | ||
int trapezoidHeight = random.nextInt(SIZE_LIMIT) + 1; | ||
return new IsoscelesTrapezoid(sideA, trapezoidSideB, trapezoidHeight, randomColor); |
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 extract random.nextInt(SIZE_LIMIT) + 1
to a separate method. And then you will be able to
return new IsoscelesTrapezoid(getRandomValue(),
getRandomValue(),
getRandomValue(),
colorSupplier.getRandomColor()
);
for (Drawable figure : figures) { | ||
figure.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.
You can draw a figure right after you've created it, eliminating the need to iterate over all the figures
} | ||
|
||
public Figure getDefaultFigure() { | ||
return new Circle(DEFAULT_CIRCLE_RADIUS, Color.WHITE); |
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.
Minor: it's better to make Color.WHITE as a constant too with a proper name to give some context as you did with radius
I'm not sure if it could be solved without interface implementation on the Figure class. But in this case, it comes to overridden methods from the interface in the Top Class, and it looks like the interface becomes useless because of method inheritance. Kinda closed circle.