-
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
Figure #1784
base: master
Are you sure you want to change the base?
Figure #1784
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 pull request! 😊 While you've made a solid effort, there are a few critical issues that need to be addressed before we can approve it:
-
Use of
super
for Fields: Directly setting fields likecolor
,name
, andarea
in subclasses usingsuper
is not ideal. It's better to initialize these fields using a constructor in theFigure
class. -
Magic Numbers: The code contains magic numbers, such as
5
innew Figure[5]
. Consider defining constants for these values to improve readability and maintainability. -
Constructor Expressions: Avoid passing expressions directly into constructors. Assign random values to variables first for better readability.
-
Typos in Output Strings: There are typos in the output strings, such as 'sq. until' instead of 'sq. units'. Please correct these to ensure clarity.
-
Code Cleanliness: Remove or uncomment any unnecessary lines to keep the code clean and maintainable.
Addressing these issues will not only improve the functionality and readability of your code but also align it with best practices. Keep up the great work, and I'm confident you'll nail it with these adjustments! 🚀
✨ 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.
public class Circle extends Figure { | ||
int radius; | ||
public Circle(int radius, String color) { | ||
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.
Directly accessing and modifying the color
field of the superclass using super
is not a good practice. Consider using a constructor in the Figure
class to initialize this field.
public Circle(int radius, String color) { | ||
super.color = color; | ||
this.radius = radius; | ||
super.name = "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.
Directly setting the name
field of the superclass using super
is not ideal. Consider using a constructor in the Figure
class to initialize this field.
super.color = color; | ||
this.radius = radius; | ||
super.name = "Circle"; | ||
super.area = Math.PI * radius * radius; |
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.
Directly setting the area
field of the superclass using super
is not ideal. Consider using a constructor in the Figure
class to initialize this field.
ColorSupplier supplier = new ColorSupplier(); | ||
Random random = new Random(); | ||
|
||
Figure[] figure = new Figure[5]; |
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 defining a constant for the number of figures (5) to avoid using magic numbers. This will improve code readability and maintainability.
Random random = new Random(); | ||
|
||
Figure[] figure = new Figure[5]; | ||
figure[0] = new Square(random.nextInt(10) + 1, supplier.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.
Avoid passing expressions directly into the constructor. Assign the random value to a variable first for better code readability.
|
||
@Override | ||
public String getFigure() { | ||
return "Figure: " + super.name + ", area: " + super.area + " sq. until, firstLeg: " + firstLeg + " until, secondLeg: " + secondLeg + " until, color: " + super.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.
There are typos in the output string: 'sq. until' should be 'sq. units', and 'until' should be 'units'.
double firstSide; | ||
public Square(int firstSide, String color){ | ||
this.firstSide = firstSide; | ||
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.
Directly accessing and modifying the color
field of the superclass using super
is not a good practice. Consider using a constructor in the Figure
class to initialize this field.
public Square(int firstSide, String color){ | ||
this.firstSide = firstSide; | ||
super.color = color; | ||
super.name = "Square"; |
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.
Directly setting the name
field of the superclass using super
is not ideal. Consider using a constructor in the Figure
class to initialize this field.
this.firstSide = firstSide; | ||
super.color = color; | ||
super.name = "Square"; | ||
super.area = firstSide * firstSide; |
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.
Directly setting the area
field of the superclass using super
is not ideal. Consider using a constructor in the Figure
class to initialize this field.
|
||
@Override | ||
public String getFigure() { | ||
return "Figure: " + super.name + ", area: " + super.area + " sq. until, side: " + firstSide + " until, color: " + super.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.
There are typos in the output string: 'sq. until' should be 'sq. units', and 'until' should be 'units'.
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.
Great job on making significant improvements to your code! 🎉 While there are still a few issues to address, such as the use of super
for field initialization and the presence of magic numbers, your efforts to enhance readability and consistency are evident.
Here are some key points to focus on for further refinement:
-
Field Initialization: Avoid using
super
to set fields likecolor
,area
, andname
directly in subclasses. Instead, consider initializing these fields in theFigure
class constructor to maintain a clean and consistent design. -
Type Consistency: Ensure that constructor parameters match the field types to avoid unnecessary casting. For example, using
double
for parameters likefirstSide
,secondSide
,firstLeg
,secondLeg
, andheight
can help maintain consistency. -
Magic Numbers: Replace magic numbers, such as
5
and6
, with named constants to improve code readability and maintainability. -
Enum Usage: Consider using the
Color
enum directly instead ofString
for color-related methods to maintain consistency and leverage the benefits of type safety.
Your dedication to improving your code is commendable. Keep up the great work, and continue to learn from these experiences to enhance your coding skills! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
public class Circle extends Figure { | ||
int radius; | ||
public Circle(int radius, String color) { | ||
super(color, Math.PI * radius * radius,"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.
Using super
to set fields like color
, area
, and name
directly is not ideal. Consider initializing these fields in the Figure
class constructor instead.
|
||
public class ColorSupplier { | ||
|
||
public String 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.
Consider changing the return type of getRandomColor()
to Color
instead of String
to maintain consistency with the Color
enum.
return color[random.nextInt(color.length)].name(); | ||
} | ||
|
||
public String getDefaultColor() { |
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 getDefaultColor()
to Color
instead of String
to maintain consistency with the Color
enum.
figure[2] = new Rectangle(value1[2], value1[2] + 10, colors[2]); | ||
figure[3] = new RightTriangle(value1[3], value1[3], colors[3]); | ||
figure[4] = new IsoscelesTrapezoid(value1[4], value1[4] + 6, value1[0], colors[4]); | ||
return figure[random.nextInt(5)]; |
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 the magic number 5
with NUMBER_OF_FIGURES
to maintain consistency and improve readability.
|
||
public Figure getDefaultFigure() { | ||
ColorSupplier supplier = new ColorSupplier(); | ||
String color = supplier.getDefaultColor(); |
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
enum directly instead of String
for color consistency.
double firstSide; | ||
double secondSide; | ||
|
||
public Rectangle(int firstSide, int secondSide, 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.
Consider using double
for the constructor parameters firstSide
and secondSide
to match the field types and avoid unnecessary casting.
double secondLeg; | ||
|
||
public RightTriangle(int firstLeg, int secondLeg, String color) { | ||
super(color, (double) (firstLeg * secondLeg) / 2, "RightTriangle"); |
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.
Using super
to set fields like color
, area
, and name
directly is not ideal. Consider initializing these fields in the Figure
class constructor instead.
double firstLeg; | ||
double secondLeg; | ||
|
||
public RightTriangle(int firstLeg, int secondLeg, 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.
Consider using double
for the constructor parameters firstLeg
and secondLeg
to match the field types and avoid unnecessary casting.
public class Square extends Figure{ | ||
double firstSide; | ||
public Square(int firstSide, String color){ | ||
super(color, firstSide * firstSide, "Square"); |
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.
Using super
to set fields like color
, area
, and name
directly is not ideal. Consider initializing these fields in the Figure
class constructor instead.
|
||
public class Square extends Figure{ | ||
double firstSide; | ||
public Square(int firstSide, 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.
Consider using double
for the constructor parameter firstSide
to match the field type and avoid unnecessary casting.
No description provided.