jv-oop-advanced-solution to check #1322
Conversation
…which implements two interfaces; add classes for different figures which extends Figure class; override constructors and methods from parent class; add ColorSupplier class & FigureSupplier class to generate figures; create an array of figures in main generate it.
…which implements two interfaces; add classes for different figures which extends Figure class; override constructors and methods from parent class; add ColorSupplier class & FigureSupplier class to generate figures; create an array of figures in main generate it.
| random.nextInt(20), random.nextInt(20), random.nextInt(20)); | ||
| break; | ||
| default: | ||
| return null; |
There was a problem hiding this comment.
I believe in the default case it is better to return defaultFigure instead of null, user does not expect null to be returned in this methid
| import java.util.Random; | ||
|
|
||
| public class FigureSupplier { | ||
| public static final int FIGURE_COUNT = 6; |
There was a problem hiding this comment.
If you use this variable in only this class, please make it private
| @@ -0,0 +1,5 @@ | |||
| package core.basesyntax; | |||
There was a problem hiding this comment.
It would be great if you will create packages and put figures in 'figure' package, suppliers to 'supplier'
| package core.basesyntax; | ||
|
|
||
| public abstract class Figure implements Drawable, AreaCalculator { | ||
| private String color; |
There was a problem hiding this comment.
It will be better to use enum Color instead of String to represent color
| WHITE, | ||
| BLACK, | ||
| PURPLE | ||
|
|
There was a problem hiding this comment.
Dont make unnecessary line breakes
|
|
||
| public class ColorSupplier { | ||
| public String getRandomColor() { | ||
| int index = new Random().nextInt(Color.values().length); |
There was a problem hiding this comment.
It makes sense to subtract Color.values().length to variable to make code more readable
|
|
||
| public class Main { | ||
| public static void main(java.lang.String[] args) { | ||
| Figure[] figures = new Figure[6]; |
There was a problem hiding this comment.
Magic number, it makes sense to reuse Figure Supplier#FIGURE_COUNT
| for (int i = 0; i < figures.length / 2; i++) { | ||
| figures[i] = figureSupplier.getRandomFigure(); | ||
| } | ||
| for (int y = figures.length / 2; y < figures.length; y++) { |
There was a problem hiding this comment.
Optional: it is common to use 'i' then 'j' as a variable name in for cycle, but you can you 'i' here as well
There was a problem hiding this comment.
Also to make code more readable it makes sense to create a method something like 'polulateFigures' or 'getFigures' and put the logic of creating and populating array there
|
|
||
| @Override | ||
| public int areaCalculator() { | ||
| return (int)Math.PI * this.radius * this.radius; |
There was a problem hiding this comment.
When you cast result of this expression to int value can be lost, so maybe it will be better to make areaCalculator return double
| package core.basesyntax; | ||
|
|
||
| public interface AreaCalculator { | ||
| int areaCalculator(); |
There was a problem hiding this comment.
Bad method name, 'calculateArea' or something like this will be better
No description provided.