Solution #5207
Solution #5207
Conversation
There was a problem hiding this comment.
Thank you for your hard work on this task! 🌟 While your code is well-structured and meets many of the task requirements, there's a critical issue with the discount logic that needs to be addressed. Currently, the function applies only one discount, either long-term or short-term, without considering the possibility of both conditions being true. This means that the long-term discount will never be applied if the short-term condition is checked afterward. Please restructure the logic to ensure the correct discount is applied based on the number of rental days. Once this is resolved, your solution will be ready for approval. 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 >= SHORT_TERM) { | ||
| return totalCost - SHORT_TERM_DISCOUNT; |
There was a problem hiding this comment.
The logic here applies the short-term discount if the number of days is greater than or equal to SHORT_TERM. However, this condition will also be true for long-term rentals (days >= LONG_TERM), which means the long-term discount will never be applied if the short-term condition is checked afterward. Consider restructuring the logic to ensure the correct discount is applied.
There was a problem hiding this comment.
Review Summary
Great job on implementing the calculateRentalCost function! 🎉 Your code meets the task requirements and constraints effectively, using descriptive names for constants and avoiding hardcoding, which enhances readability and maintainability. You've also made good use of const for immutable variables and simplified the conditional logic, which is commendable.
However, there's a small issue with the discount logic. Currently, the function applies only one discount based on the rental days, either the long-term or short-term discount, without considering scenarios where both conditions could be applicable. This means the long-term discount condition takes precedence, and the short-term discount might not be applied correctly. Consider restructuring the logic to ensure that the correct discount is applied based on the number of rental days.
Despite this, the decision is to APPROVE your Pull Request to help you move forward with your learning. Please review the comments and address this issue in future implementations to avoid similar problems. Keep up the great work, and continue to improve your code by learning from these insights! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| if (days >= LONG_TERM) { | ||
| return totalCost - LONG_TERM_DISCOUNT; | ||
| } else if (days >= SHORT_TERM) { | ||
| return totalCost - SHORT_TERM_DISCOUNT; |
There was a problem hiding this comment.
The logic here applies only one discount, either long-term or short-term, without considering the possibility of both conditions being true. This means that the long-term discount will never be applied if the short-term condition is checked afterward. Consider restructuring the logic to ensure the correct discount is applied based on the number of rental days.
No description provided.