Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because, while your calculateRentalCost function is functionally correct, it currently violates several explicit checklist requirements, which count as critical for this task. The numeric literals 40, 7, 50, 3, and 20 are used directly in expressions, which breaks the “no hard-coded literals” rule [CHECKLIST ITEM #2]; you should extract them into clearly named const values like DAILY_RATE, LONG_TERM_DAYS, LONG_TERM_DISCOUNT, SHORT_TERM_DAYS, and SHORT_TERM_DISCOUNT. The variable name result is too generic [CHECKLIST ITEM #1]; please rename it to something more descriptive such as baseCost or totalCost so the purpose is obvious from the name. Also, the value stored in result is not reassigned, so it should be declared with const instead of let to prevent accidental modification [CHECKLIST ITEM #3]. Finally, your conditional logic uses if/else if style instead of early returns, which goes against the guideline to prefer if with return and to avoid else after a return [CHECKLIST ITEMS #4 and #5]; refactor so you compute the base cost once and then use early returns for the long-term and short-term discount cases. Once you address these checklist issues, your solution will both work correctly and fully meet the project’s coding standards—your core logic is solid, so you’re very close.
✨ 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
| */ | ||
| function calculateRentalCost(days) { | ||
| // write code here | ||
| let result = days * 40; |
There was a problem hiding this comment.
This line violates checklist item #2: "Don't use literals in expressions (HARDCODE). Instead use constants explaining this values" — the daily rate 40 is used as a literal. It also violates checklist item #1: "Make names descriptive, so they explain the purpose of variables and functions." (result is vague) and checklist item #3: "Prefer const over let where possible, to avoid unintentional changes." — extract 40, 50, and 20 into named const values (for example DAILY_RATE, LONG_TERM_DISCOUNT, SHORT_TERM_DISCOUNT) and rename result to totalCost or baseCost to satisfy these checklist items .
src/calculateRentalCost.js
Outdated
| // write code here | ||
| let result = days * 40; | ||
|
|
||
| if (days >= 7) { |
There was a problem hiding this comment.
This conditional structure violates checklist item #4: "Prefer if with return over if else to simplify later conditions." and checklist item #5: "DON'T add else after if with return - the code after it won't be executed anyway." — avoid else if here. Compute baseCost then use early returns, for example: if (days >= LONG_TERM_DAYS) return baseCost - LONG_TERM_DISCOUNT; if (days >= SHORT_TERM_DAYS) return baseCost - SHORT_TERM_DISCOUNT; return baseCost; This follows the checklist guidance and removes the need for else blocks .
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your solution because all functional requirements are met: the totals are correct for the specified day counts, and you extracted the numeric literals into clearly named constants instead of hard-coding them. The remaining issues are style and checklist-alignment rather than functional blockers. Specifically, the variable name result in src/calculateRentalCost.js is generic; per the checklist [CHECKLIST ITEM #1], a name like baseCost or totalCost would better explain its purpose. Also, the discount logic currently relies on if ... else if ..., while the checklist prefers early returns [CHECKLIST ITEM #4] and avoiding else after a return [CHECKLIST ITEM #5]; refactoring to compute const baseCost = days * PRICE_PER_DAY; once and then return early for the 7+ and 3–6 day discounts would align with that guidance. As a minor improvement, once you compute a base cost and immediately return adjusted values, you can favor const over let there. Your core implementation is solid; tightening these naming and control-flow patterns will make the code cleaner and closer to the checklist for future tasks.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| function calculateRentalCost(days) { | ||
| // write code here | ||
| const pricePerDay = 40; | ||
| let result = days * pricePerDay; |
There was a problem hiding this comment.
This uses a generic name result. This violates checklist item #1: "Make names descriptive, so they explain the purpose of variables and functions." Consider renaming to totalCost or baseCost so the intent is clear.
| if (days >= longRentDays) { | ||
| result -= longRentDiscount; | ||
| } else if (days >= mediumRentDays) { | ||
| result -= mediumRentDiscount; |
There was a problem hiding this comment.
The discount logic uses if ... else if ... rather than early returns. This violates checklist item #4: "Prefer if with return over if else to simplify later conditions." and checklist item #5: "DON'T add else after if with return - the code after it won't be executed anyway." Refactor to compute the base cost once and use early returns for long-term and short-term discounts (example in review).
No description provided.