Skip to content
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

Solution #4870

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Solution #4870

wants to merge 2 commits into from

Conversation

naz4ik
Copy link

@naz4ik naz4ik commented Nov 1, 2024

No description provided.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


Hello! 😊 It looks like your implementation of the calculateRentalCost function is mostly on the right track, but there's a critical issue with the discount logic that needs to be addressed before we can approve the pull request.

Key Issue:

  • Discount Logic: The function currently applies fixed discounts for rentals of 3 days or more and 7 days or more. However, it's important to verify if this aligns with the task requirements. If the discount should be calculated differently, such as being applied per day after a certain threshold, you'll need to adjust the logic accordingly.

Please review the task requirements to ensure the discount logic is implemented as intended. Once this is resolved, your code should 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.

Comment on lines +14 to +15
if (days >= longTerm) {
return days * valueDay - longTermDiscount;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The discount logic here applies a fixed discount of 50 for rentals of 7 days or more. Ensure this aligns with the task requirements. If the discount should be applied per day after a certain threshold, the logic needs to be adjusted.

Comment on lines +18 to +19
if (days >= smallTerm) {
return days * valueDay - smallTermDiscount;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly, the discount logic for rentals of 3 days or more applies a fixed discount of 20. Verify if this is the intended behavior according to the task requirements. If the discount should be calculated differently, consider revising this logic.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a 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 implementing the calculateRentalCost function! 🎉 Your code meets the task requirements and constraints effectively. Here are some highlights and areas for improvement:

  1. Correct Implementation: The function calculates the rental cost accurately, applying discounts for rentals longer than 3 and 7 days. The logic is sound, but ensure that the discounts are not cumulative unless intended.

  2. Descriptive Naming: You've used descriptive variable names like longTerm, longTermDiscount, smallTerm, smallTermDiscount, and valueDay, which makes the code easy to read and maintain.

  3. Use of Constants: Good job using constants instead of hardcoded values, which enhances the code's flexibility and readability.

  4. Variable Declarations: You've used const for variable declarations, which is a best practice for values that do not change.

  5. Simplified Structure: The function structure avoids unnecessary else statements, making the code cleaner.

Areas for Improvement

  • Discount Logic: Ensure that the discounts are applied correctly. If the intention is to apply only the larger discount for rentals longer than 7 days, your current logic is correct. If the discounts should be cumulative, consider adjusting the logic.

This decision to approve is to help you move forward with your learning. Please review the comments to improve your code further. Remember, addressing critical issues in the future will help you avoid similar problems and enhance your coding skills. 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! ✨

Comment on lines +14 to +20
if (days >= longTerm) {
return days * valueDay - longTermDiscount;
}

if (days >= smallTerm) {
return days * valueDay - smallTermDiscount;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current logic applies a discount for rentals longer than 7 days first, and then for rentals longer than 3 days. This is correct, but ensure that the discounts are not cumulative. If the intention is to apply only one discount (the larger one), the current implementation is correct. If the discounts should be cumulative, the logic needs to be adjusted.

Copy link

@anastasiiavorobiova anastasiiavorobiova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! Almost done!

src/calculateRentalCost.js Outdated Show resolved Hide resolved
Co-authored-by: anastasiiavorobiova <135232001+anastasiiavorobiova@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants