-
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
Home Work solution jv-oop-advanced #1740
base: master
Are you sure you want to change the base?
Home Work solution jv-oop-advanced #1740
Conversation
…plier.java, interface Colors.java, interface Drawable.java, class Figure.java, class FigureSupplier.java, interface Figures.java, class IsoscelesTrapezoid.java, class Rectangle.java, class RightTriangle.java, class Square.java; Changed HelloWorld.java
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 your implementation is mostly solid, there are a few critical issues that need to be addressed before we can approve it:
-
Non-English Text in
drawFigure()
Methods: Several classes (Circle
,RightTriangle
,Square
,IsoscelesTrapezoid
,Rectangle
) contain non-English text in theirdrawFigure()
methods. It's important to use English for all messages to ensure consistency and universality across the codebase. -
Returning
null
inFigureSupplier
: ThegetRandomFigure()
method in theFigureSupplier
class returnsnull
in thedefault
case. This is not recommended as it can lead toNullPointerExceptions
. Consider handling this case by throwing an exception or returning a default figure. -
Use of
toString()
for Enum Constants: In theColorSupplier
class, replacetoString()
withname()
for getting the string representation of enum constants. This aligns with best practices and ensures reliability. -
Hardcoded Pi Value: The
Circle
class uses a hardcoded value of3.14
for Pi. Replace this withMath.PI
for better precision in calculations.
These changes are necessary to ensure the code meets the core requirements and maintains high quality. Once these issues are resolved, your pull request will be ready for approval. Keep up the great work, and don't hesitate to reach out if you have any questions! 😊
✨ 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 double getS() { | ||
return 3.14 * Math.pow(this.radius, 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.
Consider using Math.PI
instead of 3.14
for better precision in calculating the area of the circle.
System.out.println("Коло { радіус = " + this.radius + ", площа = " | ||
+ getS() + ", колір - " + 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.
The drawFigure()
method contains non-English text. According to the checklist, you should use English for messages to ensure the code is universal and consistent.
public String getRandomColor() { | ||
int index = new Random().nextInt(Colors.values().length); | ||
Colors color = Colors.values()[index]; | ||
return color.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.
Instead of using toString()
, use name()
to get the string representation of the enum constant. This is a more reliable method as per the checklist guidelines.
int index = random.nextInt(Figures.values().length); | ||
Figures figure = Figures.values()[index]; | ||
|
||
switch (figure.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.
Instead of using toString()
, use name()
to get the string representation of the enum constant. This is a more reliable method as per the checklist guidelines.
random.nextDouble() * 100, | ||
random.nextDouble() * 100); | ||
default: | ||
return null; |
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.
Returning null
from a method is considered a bad practice. Consider handling the default case differently, such as by throwing an exception or returning a default figure.
System.out.println("Рівнобічна трапеція { основа1 = " + this.base1 | ||
+ ", основа2 = " + this.base2 | ||
+ ", висота = " + this.height + ", площа = " + getS() | ||
+ "колір - " + 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.
The drawFigure()
method contains non-English text. According to the checklist, you should use English for messages to ensure the code is universal and consistent.
+ ", висота = " + this.height + ", площа = " + getS() | ||
+ "колір - " + 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 space before 'колір'. Ensure proper spacing for readability.
System.out.println("Прямокутник { сторона1 = " + this.side1 + ", сторона2 = " + this.side2 | ||
+ ", площа = " + getS() + ", колір - " + 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.
The drawFigure()
method contains non-English text. According to the checklist, you should use English for messages to ensure the code is universal and consistent.
System.out.println("Прямокутний трикутник { катет1 = " + this.leg1 + ", катет2 = " | ||
+ this.leg2 + ", гіпотенуза = " + this.hypotenusa | ||
+ ", площа = " + getS() | ||
+ ", колір - " + 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.
The drawFigure()
method contains non-English text. According to the checklist, you should use English for messages to ensure the code is universal and consistent.
System.out.println("Квадрат { сторона = " + this.side + ", площа = " | ||
+ getS() + ", колір - " + 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.
The drawFigure()
method contains non-English text. According to the checklist, you should use English for messages to ensure the code is universal and consistent.
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.
Review Summary
Great job on addressing the previous issues and aligning your code with the task requirements! 🎉 Your implementation now meets the necessary standards, and the improvements you've made are commendable. Here are some key points and suggestions for further enhancement:
-
Non-English Text in
drawFigure()
Methods: You've successfully updated thedrawFigure()
methods in all relevant classes to use English text, ensuring consistency and universality across the codebase. -
Returning
null
inFigureSupplier
: ThegetRandomFigure()
method no longer returnsnull
. Instead, it throws anIllegalStateException
in thedefault
case, which is a better practice. -
Use of
toString()
for Enum Constants: TheColorSupplier
class now usesname()
instead oftoString()
for getting the string representation of enum constants, aligning with best practices. -
Hardcoded Pi Value: The
Circle
class now usesMath.PI
instead of a hardcoded value for Pi, ensuring better precision in calculations. -
Loop Condition: Consider using
i < 3
instead ofi <= 2
for the loop condition inHelloWorld.java
to follow the conventional pattern and avoid confusion. -
Consistency in Enum Usage: Ensure consistency by using
Colors.WHITE.name()
instead ofColors.WHITE.toString()
.
Although there are minor issues, the decision to approve is made to help you move forward with your learning. Please review the comments to further improve your code. Remember, addressing these points will enhance your coding skills and prevent similar issues in the future. Keep up the great work and continue learning from your experiences! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
} | ||
|
||
public Figure getDefaultFigure() { | ||
return new Circle(Colors.WHITE.toString(), 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 Colors.WHITE.name()
instead of Colors.WHITE.toString()
for consistency with the rest of the codebase.
|
||
for (int i = 0; i <= 2; i++) { |
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 loop condition i <= 2
should be i < 3
to avoid confusion and ensure consistency with the typical loop pattern. Both conditions are functionally equivalent here, but using i < 3
is more conventional.
Added interface CalculateArea.java, class Circle.java, class ColorSupplier.java, interface Colors.java, interface Drawable.java, class Figure.java, class FigureSupplier.java, interface Figures.java, class IsoscelesTrapezoid.java, class Rectangle.java, class RightTriangle.java, class Square.java; Changed HelloWorld.java