Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your function correctly calculates the rental cost, which is a great start! However, I'm requesting a few changes to align with the project's coding standards. The primary issue is the use of 'magic numbers' like 6 and 2 in your if conditions. Please replace these with descriptive named constants, as this is a requirement from our checklist [CHECKLIST ITEM #2]. Additionally, the variable names for discounts, such as saleMost3Days, could be clearer. Consider a name like DISCOUNT_FOR_3_OR_MORE_DAYS to better describe its purpose [CHECKLIST ITEM #1]. Making these adjustments will significantly improve your code's readability.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
src/calculateRentalCost.js
Outdated
| const saleMost3Days = 20; | ||
| const saleMost7Days = 50; |
There was a problem hiding this comment.
This violates checklist item #1: 'Make names descriptive'. The names saleMost3Days and saleMost7Days can be a bit confusing. A name like DISCOUNT_FOR_3_OR_MORE_DAYS would more clearly describe that this is a discount applied for rentals of 3 days or longer.
src/calculateRentalCost.js
Outdated
|
|
||
| const rentalSum = rentalPrice * days; | ||
|
|
||
| if (days > 6) { |
There was a problem hiding this comment.
This condition uses a 'magic number' (6), which violates checklist item #2: 'Don't use literals in expressions (HARDCODE)'. Please use a named constant to represent the rental duration threshold. For example, you could create a constant for 7 days and check if days >= YOUR_CONSTANT_NAME. This makes the logic easier to understand at a glance.
src/calculateRentalCost.js
Outdated
| return rentalSum - saleMost7Days; | ||
| } | ||
|
|
||
| if (days > 2) { |
There was a problem hiding this comment.
This is another 'magic number' (2), which violates checklist item #2. Similar to the check above, please replace it with a descriptive named constant.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great work on this! Your function correctly calculates the rental cost, and you've successfully replaced the magic numbers with named constants. Since the logic is sound and meets all the requirements, I am approving your solution.
As a final tip for improving readability, it's a strong convention to name constants using UPPER_SNAKE_CASE. This makes them instantly recognizable. For example:
const COST_PER_DAY = 40;
const LONG_TERM_DISCOUNT = 50;
const MEDIUM_TERM_DISCOUNT = 20;This is a minor style point but a very useful practice to adopt. Keep up the excellent progress!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| const rentalPrice = 40; | ||
| const discountFor3OrMostday = 20; | ||
| const discountFor7OrMostday = 50; | ||
| const rentalDayForDiscount20 = 3; | ||
| const rentalDayForDiscount50 = 7; |
There was a problem hiding this comment.
It's great that you've replaced magic numbers with named constants! To make this even more readable and align with the checklist's examples, consider naming constants based on the concept they represent, rather than their value.
Also, there's a small typo in ...OrMostday; it should likely be ...OrMoreDays.
Here's a suggestion for a more conventional naming scheme:
const COST_PER_DAY = 40;
const LONG_TERM_DISCOUNT = 50;
const MEDIUM_TERM_DISCOUNT = 20;
const LONG_TERM_DAYS_THRESHOLD = 7;
const MEDIUM_TERM_DAYS_THRESHOLD = 3;
No description provided.