-
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
Solved tasks from ReadMe file #1292
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, but a few comments need to be fixed.
@@ -0,0 +1,10 @@ | |||
package core.basesyntax.enums; | |||
|
|||
public enum FigureColor { |
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.
Let's simplify. Just Color
.
package core.basesyntax.figures; | ||
|
||
public interface Drawable { | ||
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.
Just draw
, maybe would be better?
protected int area; | ||
|
||
@Override | ||
public 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.
You can make thin class abstract and not override a method with empty implementation.
for (int i = 0; i < ARRAY_LENGTH; i++) { | ||
if (i < ARRAY_LENGTH / 2) { | ||
figures[i] = figureSupplier.getRandomFigure(); | ||
continue; |
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.
Do not use continue
, use just if {} else {}
.
|
||
public class ColorSupplier { | ||
private final Random random = new Random(); | ||
private final int randomIndex = random.nextInt(FigureColor.values().length); |
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 random index will be created once on creating ColorSupplier.
I think you should have a random index on every getRandomColor
method call.
if (randomIndex == 0) { | ||
return new Circle(randomColor, randomProperty1); | ||
} | ||
if (randomIndex == 1) { | ||
return new IsoscelesTrapezoid(randomColor, | ||
randomProperty1, randomProperty2, randomProperty3); | ||
} | ||
if (randomIndex == 2) { | ||
return new Rectangle(randomColor, randomProperty1, randomProperty2, randomProperty3); | ||
} | ||
if (randomIndex == 3) { | ||
return new RightTriangle(randomColor, randomProperty1, randomProperty2); | ||
} |
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 custom switch. Lets use default java switch.
int randomProperty1 = random.nextInt(MAX_VALUE_OF_PROPERTY); | ||
int randomProperty2 = random.nextInt(MAX_VALUE_OF_PROPERTY); | ||
int randomProperty3 = random.nextInt(MAX_VALUE_OF_PROPERTY); |
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 it is redundant. You can create object with the text logic:
return new Circle(colorSupplier.getRandomColor(), getRandomValue());
int randomProperty1 = random.nextInt(MAX_VALUE_OF_PROPERTY); | ||
int randomProperty2 = random.nextInt(MAX_VALUE_OF_PROPERTY); | ||
int randomProperty3 = random.nextInt(MAX_VALUE_OF_PROPERTY); | ||
String randomColor = new ColorSupplier().getRandomColor(); |
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.
Think about this.
this.sideEdge = sideEdge; | ||
this.topSide = topSide; | ||
this.downSide = downSide; | ||
this.area = ((topSide * downSide) / 2) * sideEdge; |
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.
Does that formula calculate area of the isosceles trapezoid?
this.firstSide = firstSide; | ||
this.secondSide = secondSide; | ||
this.thirdSide = thirdSide; | ||
this.area = firstSide * secondSide * thirdSide; |
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.
How does Rectangle translate?
How do we calculate it's 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.
Well done!
No description provided.