-
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
created several Figure and color classes and their suppliers #1729
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.
Thank you for your hard work on this project! While your code is mostly on the right track, there are a few critical issues that need to be addressed before we can approve the pull request:
-
Area Calculation Errors: The
calculateArea()
methods in both theCircle
andRightTriangle
classes have inaccuracies. For theCircle
, castingMath.PI
to anint
leads to a loss of precision. Similarly, theRightTriangle
area calculation does not use the correct formula. Consider usingdouble
for more precise calculations. -
Enum Usage: It's recommended to use the
Color
andFigureName
enums directly instead of converting them toString
. This will enhance type safety and align with the intended use of enums. -
Return Type in
ColorSupplier
: ThegetRandomColor
method currently returns aString
, but it would be more appropriate to return aColor
type directly. This change will better utilize theColor
enum.
These changes are necessary to ensure the functionality and accuracy of your code. Keep up the great work, and I'm confident you'll address these issues swiftly! 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
🔄 Fix the issues, commit the changes, and then re-request my review using the Re-request review
button in the Reviewers tab.
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.
Your code has been reviewed and approved! 🎉 While there are some areas for improvement, the overall implementation meets the task requirements. Here are some key points to consider for future enhancements:
-
Magic Numbers: Avoid using magic numbers directly in your code. Define them as constants for better readability and maintainability.
-
FigureSupplier Instance: Create the
FigureSupplier
instance once before the loop in theMain
class to optimize performance. -
Constructor Expressions: Assign expressions to variables before passing them to constructors to enhance code clarity.
-
Class-Level Variables: Consider making the
Random
instance inFigureSupplier
a class-level variable to avoid multiple instantiations. -
Abstract Class: If
Figure
is not meant to be instantiated directly, consider making it an abstract class. -
Enum Usage: Use the
Color
andFigureName
enums directly to maintain type safety and consistency. -
Precision in Calculations: Use
double
for calculations incalculateArea
methods to prevent precision loss. -
Formatting: Ensure proper formatting in
toString
methods and remove any unnecessary characters like extra commas.
These improvements will help you write cleaner and more efficient code. Keep up the great work, and continue learning from these suggestions! 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
public int calculateArea() { | ||
return (int) (radius * radius * Math.PI); |
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 calculateArea
method returns an int
, but the area of a circle is a double value due to the use of Math.PI
. Consider changing the return type to double
to avoid loss of precision.
public String getRandomColor() { | ||
Color[] values = Color.values(); | ||
int randomIndex = new Random().nextInt(values.length); | ||
return values[randomIndex].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.
Consider changing the return type of getRandomColor
from String
to Color
to maintain consistency with the Color
enum. This will allow you to work directly with the Color
type instead of converting it to a String
.
@@ -0,0 +1,51 @@ | |||
package core.basesyntax; | |||
|
|||
public class Figure implements 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.
Consider making Figure
an abstract class if it is intended to serve as a base class for other specific figure types like Circle
. This would prevent direct instantiation of Figure
and emphasize that it is meant to be extended.
case 2 -> randomFigure = getRandomRectangle(); | ||
case 3 -> randomFigure = getRightTriangle(); | ||
case 4 -> randomFigure = getRandomSquare(); | ||
default -> randomFigure = 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.
The default case in the switch statement creates a new Figure
object. If Figure
is intended to be an abstract class or should not be instantiated directly, consider removing this default case or handling it differently.
} | ||
|
||
public Figure getDefaultFigure() { | ||
return new Circle(Color.WHITE.name(), FigureName.CIRCLE.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.
Consider using the Color
and FigureName
enums directly instead of calling name()
. This will maintain type safety and consistency with the enum usage.
public static void main(String[] args) { | ||
Figure[] figures = new Figure[6]; | ||
for (int i = 0; i < 3; i++) { | ||
figures[i] = new FigureSupplier().getRandomFigure(); |
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.
Ensure that FigureSupplier.getRandomFigure()
does not return a direct instance of Figure
if Figure
is intended to be a base class or abstract. This aligns with the suggestion to make Figure
an abstract class.
private int firstLag; | ||
private int secondLag; |
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 names firstLag
and secondLag
might be confusing. Consider renaming them to width
and height
or length
and breadth
for better clarity and understanding.
public int calculateArea() { | ||
return (int) (Math.pow(firstLag, 2) * (Math.sqrt(3)) / 4); |
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 formula used in calculateArea
is for an equilateral triangle, not a right triangle. For a right triangle, the area should be calculated as (base * height) / 2
. Ensure that the correct dimensions are used for a right triangle.
package core.basesyntax; | ||
|
||
public class RightTriangle extends Figure { | ||
private int firstLag; |
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 firstLag
might be confusing. Consider renaming it to base
or height
for better clarity and understanding, especially since this is a right triangle.
package core.basesyntax; | ||
|
||
public class Square extends Figure { | ||
private int firstLag; |
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 firstLag
might be confusing. Consider renaming it to side
for better clarity and understanding, as it represents the side length of the square.
No description provided.