-
Notifications
You must be signed in to change notification settings - Fork 4.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
calculate rental amount #4853
base: master
Are you sure you want to change the base?
calculate rental amount #4853
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 task! 😊 While your solution is on the right track, there's a critical issue with the discount logic that needs to be addressed. Currently, both discounts can be applied simultaneously, which is not the intended behavior. You should adjust the logic to ensure only the highest applicable discount is applied. Additionally, consider replacing hardcoded literals with constants for better readability and maintainability, and use const
instead of let
where possible. Once these changes are made, your code will be much stronger! Keep up the great work! 💪
✨ 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.
src/calculateRentalCost.js
Outdated
if (days >= 3) { | ||
return totalSum - 20; |
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.
Logical issue: The current logic applies a $20 discount for rentals of 3 or more days, even if a $50 discount for 7 or more days has already been applied. Consider using an else if
statement here to ensure only one discount is applied.
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! Let's improve your code a bit)
src/calculateRentalCost.js
Outdated
for (let day = 1; day <= days; day++) { | ||
totalSum = totalSum + pricePerDay; | ||
} |
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 think you can use multiplying for this)
src/calculateRentalCost.js
Outdated
} | ||
|
||
if (days >= 7) { | ||
return totalSum - 50; |
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.
looks like magic number, create variable for all discount
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.
Looks good
No description provided.