-
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
Added necessary Classes/Test passed #1508
base: master
Are you sure you want to change the base?
Conversation
|
||
public static String getRandomColor() { | ||
|
||
String[] colors = { |
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.
These colors are more of a constant so putting them on method level is a bad idea. Additionally, you could extract them to enum.
"magenta", "brown" | ||
}; | ||
|
||
Random random = new Random(); |
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 make random a variable - store it as a constant.
this.color = "white"; | ||
} | ||
|
||
public interface Drawable { |
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.
Why did you put this interface here? You've already created it in separate file.
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.
Not fixed
void draw(); | ||
} | ||
|
||
public interface Colorful { |
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.
Why did you put this interface here? You've already created it in separate file.
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.
Not fixed
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 should be provided by interface implementation.
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.
Not fixed
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.
Not fixed for the second time
@@ -5,4 +5,5 @@ | |||
*/ | |||
public class HelloWorld { | |||
|
|||
|
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.
Unnecessary change.
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.
Not fixed
package core.basesyntax; | ||
|
||
public class IsoscelesTrapezoid extends Figure { | ||
private final double firstLeg; |
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.
These names tell nothing about the sides they are referring to. Use:
- topSize
- bottomSide
- 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.
Not fixed
package core.basesyntax; | ||
|
||
public class Rectangle extends Figure { | ||
private final double firstLeg; |
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.
Same here - width and height are more appropriate.
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.
Not fixed
package core.basesyntax; | ||
|
||
public class Square extends Figure { | ||
private final double firstLeg; |
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.
side
, there are no other legs except the first one.
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.
Not fixed
@@ -0,0 +1,5 @@ | |||
package core.basesyntax; | |||
|
|||
public interface Colorful { |
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 interface is unnecessary.
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.
not fixed
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.
Missed a lot of comments - please check if all fixes have been applied BEFORE re-requesting the review.
private final double radius; | ||
|
||
public Circle(Color color, double radius) { | ||
super(String.valueOf(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.
Why did you keep a String type in base class, when you're using a Color instance everywhere else? Please fix this in base class.
@@ -0,0 +1,5 @@ | |||
package core.basesyntax; | |||
|
|||
public interface Colorful { |
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.
not fixed
this.color = "white"; | ||
} | ||
|
||
public interface Drawable { |
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.
Not fixed
void draw(); | ||
} | ||
|
||
public interface Colorful { |
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.
Not fixed
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.
Not fixed
public static Figure getRandomFigure() { | ||
int randomNumber = (int) (Math.random() * 5); | ||
|
||
switch (randomNumber) { |
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.
Not fixed
@@ -5,4 +5,5 @@ | |||
*/ | |||
public class HelloWorld { | |||
|
|||
|
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.
Not fixed
package core.basesyntax; | ||
|
||
public class IsoscelesTrapezoid extends Figure { | ||
private final double firstLeg; |
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.
Not fixed
package core.basesyntax; | ||
|
||
public class Rectangle extends Figure { | ||
private final double firstLeg; |
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.
Not fixed
package core.basesyntax; | ||
|
||
public class Square extends Figure { | ||
private final double firstLeg; |
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.
Not fixed
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 see that you ignored some of my comments for two iterations straight so I will stop reviewing this PR discard the request - please go through the whole Conversation
tab and check for all requested changes.
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.
Not fixed for the second time
public static Figure getRandomFigure() { | ||
int randomNumber = (int) (Math.random() * 5); | ||
|
||
switch (randomNumber) { |
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.
Not fixed for the second time
…t from 0 so it can be a little changed
…t from 0 so it can be a little changed
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.
During the previous iteration, I asked you to carefully check all comments.
I won't review your code until you apply all requested changes.
public class ColorSupplier { | ||
|
||
public String getRandomColor() { | ||
int index = new Random().nextInt(Color.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.
I've already mentioned 2 times to make Random
instance a constant - stopping review here.
…ecided to write all code from 0. I know you could be upset because it may be looking like i'm ignoring your comments but plis keep in mide i'm just starting my life in programing and to be honest im not exacly sure what am i sometimes dooing in Java. I hope after i wrote all code from 0 that Random variable was the only problem in my code. I'v also checked again all your comments and i realy dont see any of bugs dubled. Mayby there is some but keep in mind when im sending code to you i'm trying my best to make it good for you. To my defend its first time when im using github. Using it i mean it like making forks or reading comments after first time when i was sending code of this quest so i didnt know where could be some clue. Now i made some reserch od it and i think that i found all problems and i fixed them
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.
Changes required - one change was requested 3 times already.
protected String color; | ||
|
||
public Figure() { | ||
ColorSupplier color = new ColorSupplier(); |
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're not supposed to set a random color here, but if you did, the ColorSupplier
instance should be a constant as well.
|
||
public Figure() { | ||
ColorSupplier color = new ColorSupplier(); | ||
this.color = color.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.
Why are you assigning random color by default here? Use constructor to allow passing the value while creating an object.
@@ -0,0 +1,5 @@ | |||
package core.basesyntax; | |||
|
|||
public interface FigureArea { |
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 is not a valid name for interface in Java. It should be an adjective or a meaningful noun - your figure is not a figure area.
int a = RANDOM.nextInt(10) + 2; | ||
int b = RANDOM.nextInt(10) + 2; | ||
int c = RANDOM.nextInt(10) + 2; |
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 create these values ahead - most them won't be always used. Do it on figures constructors calls. Also, put numeric values in constants (e.g. MAX_LENGTH)
} | ||
|
||
public Figure getDefaultFigure() { | ||
return new Circle(10, "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.
You've created an enum for colors, why not use it here?
this.color = color.getRandomColor(); | ||
} | ||
|
||
abstract String getFigureInfo(); |
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.
Not fixed for a THIRD time!
this.radius = radius; | ||
} | ||
|
||
public Circle(int radius, String 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.
The color is supposed to be an enum, for all figures.
package core.basesyntax; | ||
|
||
public abstract class Figure implements FigureArea { | ||
protected String 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.
You've created an enum for colors - why you didn't use it here??
…to allow passing value. abstract String getFigureInfo has been moved to CalculatableArea (renamed interfase) and that was the think i missed so many times and I'mso sory for it. FiguraArea renemed to CalculatableArea to make code clean. Circlie.java got a color from enum so removed wrong string from there same think made in Figure.java The bigest issue for me is FigureSupplier.java I'm not sure i understand it correctly I changed the color of DeafultFigure to white from enum so now its using it but im not sure have i done right about random values. If its still wrong plis give me some more clues
…ecided to write all code from 0. I know you could be upset because it may be looking like i'm ignoring your comments but plis keep in mide i'm just starting my life in programing and to be honest im not exacly sure what am i sometimes dooing in Java. I hope after i wrote all code from 0 that Random variable was the only problem in my code. I'v also checked again all your comments and i realy dont see any of bugs dubled. Mayby there is some but keep in mind when im sending code to you i'm trying my best to make it good for you. To my defend its first time when im using github. Using it i mean it like making forks or reading comments after first time when i was sending code of this quest so i didnt know where could be some clue. Now i made some reserch od it and i think that i found all problems and i fixed them. I'v also seperete CalculetableArea and Figure info for 2 difrent interfaces cuz of some problems in StructureTests
…ecided to write all code from 0. I know you could be upset because it may be looking like i'm ignoring your comments but plis keep in mide i'm just starting my life in programing and to be honest im not exacly sure what am i sometimes dooing in Java. I hope after i wrote all code from 0 that Random variable was the only problem in my code. I'v also checked again all your comments and i realy dont see any of bugs dubled. Mayby there is some but keep in mind when im sending code to you i'm trying my best to make it good for you. To my defend its first time when im using github. Using it i mean it like making forks or reading comments after first time when i was sending code of this quest so i didnt know where could be some clue. Now i made some reserch od it and i think that i found all problems and i fixed them. I'v also seperete CalculetableArea and Figure info for 2 difrent interfaces cuz of some problems in StructureTests
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.
Once again - you've missed fixes - go through all past comments (OUTDATED AS WELL since you haven't applied them) and see which are missing in your implementation.
As I mentioned before - I won't be checking your code until all requested changes are applied.
@@ -0,0 +1,5 @@ | |||
package core.basesyntax; | |||
|
|||
public interface CalculatableArea { |
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.
Like I've mentioned in previous comment, your Figure is neither FigureArea nor CalculatableArea - but could be for example "AreaCalculator" - please give your classes a meaningful names.
|
||
public Figure getRandomFigure() { | ||
int ran = RANDOM.nextInt(FIGURE_COUNT); | ||
int a = RANDOM.nextInt(MAX_LENGTH - MIN_LENGTH + 1) + MIN_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.
I don't understand what is happening here - why not just use RANDOM.nextInt(MAX_LENGTH + 1)? Do we need MIN_LENGTH?
package core.basesyntax; | ||
|
||
public abstract class Figure implements FigureInfo, CalculatableArea { | ||
private static final ColorSupplier colorSupplier = new ColorSupplier(); |
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.
Not fixed: #1508 (comment)
case (2): | ||
return new RightTriangle(a, b); | ||
case (3): | ||
return new Circle(a, 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.
Why you allow to pass color in constructor for Circle but not the rest of figures?
int ran = RANDOM.nextInt(FIGURE_COUNT); | ||
int a = RANDOM.nextInt(MAX_LENGTH - MIN_LENGTH + 1) + MIN_LENGTH; | ||
int b = RANDOM.nextInt(MAX_LENGTH - MIN_LENGTH + 1) + MIN_LENGTH; | ||
int c = RANDOM.nextInt(MAX_LENGTH - MIN_LENGTH + 1) + MIN_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.
Not fixed: #1508 (comment)
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 haven't added any changes since the last mentor's comments, didn't you forget to push them before you asked to check the changes again?
…ry for that. I still dont know what to do about that colors in Circle i just do not understand what should i change. Could you help me on this? I hope i fixed all bugs that nowickimj showed but if not i just couldnt find them now. Like i said before i wrote this code from 0 last time and im just trying to avoid all bugs but I have changed some Class names etc. so it should be bether now
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.
See some advice.
If you don't understand something or don't know how to do it, describe your problem in the chat on the Mate platform or sign up for Q&A for help.
|
||
@Override | ||
public double getArea() { | ||
return (firstBase * secondBase) * heigh / 2; |
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.
return (firstBase * secondBase) * heigh / 2; | |
return (firstBase + secondBase) * heigh / 2; |
|
||
public abstract class Figure implements FigureInfo, AreaCalculator { | ||
private static final ColorSupplier colorSupplier = new ColorSupplier(); | ||
protected 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.
protected Color color; | |
private Color color; | |
public Rectangle(int base, int side) { | ||
this.base = base; | ||
this.side = side; | ||
} |
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 constructor is redundant, please remove it from all classes that extends Figure
(constructors without color
)
public Rectangle(int base, int side, Color color) { | ||
this.base = base; | ||
this.side = side; | ||
super.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.
It should be like this:
public Rectangle(int base, int side, Color color) { | |
this.base = base; | |
this.side = side; | |
super.color = color; | |
} | |
public Rectangle(int base, int side, Color color) { | |
super(color); | |
this.base = base; | |
this.side = side; | |
} |
Please do it in all classes that extends 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.
Let's refactor this class to this:
public abstract class Figure implements FigureInfo, AreaCalculator {
private Color color;
public Figure(Color color) {
this.color = color;
}
public Color getColor() {
return color;
}
}
|
||
public class ColorSupplier { | ||
|
||
static final Random RANDOM = new Random(); |
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.
static final Random RANDOM = new Random(); | |
private static final Random RANDOM = new Random(); |
public String getRandomColor() { | ||
int index = RANDOM.nextInt(Color.values().length); | ||
return Color.values()[index].toString(); | ||
} |
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.
public String getRandomColor() { | |
int index = RANDOM.nextInt(Color.values().length); | |
return Color.values()[index].toString(); | |
} | |
public Color getRandomColor() { | |
int index = RANDOM.nextInt(Color.values().length); | |
return Color.values()[index]; | |
} |
int c = RANDOM.nextInt(MAX_LENGTH + 1); | ||
switch (ran) { | ||
case (0): | ||
return new Square(a); |
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.
In all cases you should pass random size and random color, so create and use ColorSupplier
, For example like this (You also need random here):
private static final ColorSupplier colorSupplier = new ColorSupplier();
private static final Random random = new Random();
private static final int FIGURE_COUNT = 5;
private static final int MIN_LENGTH = 2;
private static final int MAX_LENGTH = 10;
… this problem. I hope i didnt miss any sugestions. My code should have all sugested tips included. The only think im not sure is FigureSupplier.class but if it needs any changes plis let me know!
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 on implementing requested changes. You can read my comments and fix some problems, but overall solution is good and I am accepting your PR.
return "Figure: Circle, area: " + getArea() + " sq. units, radius: " | ||
+ radius + " units, color: " + getColor(); | ||
} | ||
/* Wzór na pole koła pi*r*r */ |
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.
The comment is in Polish and might not be understood by all developers. Consider translating it to English for consistency and clarity.
|
||
public class Main { | ||
public static void main(String[] args) { | ||
FigureSupplier figureIndex = new FigureSupplier(); |
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.
The variable name 'figureIndex' is misleading. It suggests an index or position rather than a supplier. Consider renaming it to 'figureSupplier' for clarity.
return "Figure: Square, area: " + getArea() + " sq. units, side: " | ||
+ side + " units, color: " + getColor(); | ||
} | ||
/* Wzór na pole kwadratu a*a */ |
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.
The comment on line 21 is in Polish. Consider translating it to English for consistency and to ensure that all developers can understand it.
package core.basesyntax; | ||
|
||
public class Square extends Figure { | ||
private int side; |
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.
The 'side' attribute should be validated to ensure it is a positive integer. Consider adding a check in the constructor to throw an IllegalArgumentException if 'side' is less than or equal to zero.
@@ -0,0 +1,13 @@ | |||
package core.basesyntax; | |||
|
|||
public enum 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.
Consider adding a method to get a random color from the enum. This would be useful for the ColorSupplier class to generate random colors.
|
||
@Override | ||
public String getFigureInfo() { | ||
return "Figure: IsoscelesTrapezoid, area: " + getArea() + " sq. units, firstBase: " |
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.
The method 'getFigureInfo' should use consistent formatting for the output string. Consider adding commas or semicolons between the attributes for better readability.
@Override | ||
public String getFigureInfo() { | ||
return "Figure: RightTriangle, area: " + getArea() + " sq. units, base: " | ||
+ base + " heigh " + heigh + " units, color: " + getColor(); |
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.
There is a missing colon after 'heigh' in the string concatenation. It should be 'height: ' instead of 'heigh '.
public class IsoscelesTrapezoid extends Figure { | ||
private int firstBase; | ||
private int secondBase; | ||
private int heigh; |
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.
There is a typo in the variable name 'heigh'. It should be 'height'. This will improve code readability and prevent potential confusion.
|
||
@Override | ||
public double getArea() { | ||
return (base * heigh) / 2; |
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.
The calculation for the area should be cast to double to ensure precision, as integer division may lead to loss of precision. Consider using '(double) base * heigh / 2' to ensure the result is a double.
|
||
public class RightTriangle extends Figure { | ||
private int base; | ||
private int heigh; |
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.
There is a typo in the variable name 'heigh'. It should be 'height'. This typo is repeated in the constructor and other parts of the code.
No description provided.