add solution#514
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there are still functional issues that will break expected behavior in common scenarios. The main blocker is that when zero or one CLI argument is provided (around the process.argv handling in src/app.js), you log an error but still call copy(source, destination) with undefined values; this can cause extra, unintended errors instead of cleanly exiting once invalid arguments are detected. You should add an early return or process.exit(1) immediately after logging the argument error so the program stops before calling copy.
A second important issue is error output noise when handling directories: after detecting EISDIR, you log a helpful message but then also print the raw error object, which can make the CLI output differ from what tests expect; instead, log only a single clear, user-facing message for this case. Also, your message about arguments currently says something like “Only one argument is provided,” which is misleading when no arguments are given; replace this with a more accurate message indicating that both source and destination paths are required. On the positive side, your core copying logic and same-path guard look good and are close to the requirements. Once you fix the argument control flow and simplify the directory error output, your solution should be in great shape.
✨ 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
| if (!source || !destination) { | ||
| console.error('Only one argument is provided'); | ||
| } |
There was a problem hiding this comment.
After logging the error about invalid arguments, the script still proceeds and later calls copy with possibly undefined paths; add an early return or process.exit to stop execution once you detect missing arguments.
| if (err.code === 'EISDIR') { | ||
| console.error('You try to use a directory'); | ||
| } | ||
| console.error(err); |
There was a problem hiding this comment.
You always log the raw error object after handling EISDIR; this might produce extra output beyond the specific message, which can conflict with a "plain" CLI and test expectations. Consider limiting output to a clear, user-friendly error message.
| } | ||
|
|
||
| if (!source || !destination) { | ||
| console.error('Only one argument is provided'); |
There was a problem hiding this comment.
The argument error message implies exactly one argument is provided, but this branch also runs when zero arguments are given; consider using a more accurate message for missing source or destination.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your solution is approved because all core functional requirements are met: the app correctly copies files, no-ops when source and destination are the same, exits early when arguments are invalid, and handles directory errors with a clear EISDIR message instead of dumping the raw error. The only remaining issues are around user-facing error message clarity, which are important but not critical blockers for functionality.
To improve, please adjust the messages in src/app.js where you handle invalid argument counts: when both source and destination are missing (e.g., around the branch handling process.argv.length <= 2), prefer something like “Both source and destination paths are required.” instead of “Zero argument is provided” so users immediately know what to supply. Similarly, when exactly one argument is given (the branch around process.argv.length === 3), use a message such as “Please provide both source and destination file paths.” instead of “Only one argument is provided”, which is less descriptive and not very user-focused. These changes will better align your messages with the requirement to clearly state that two paths are needed and will make your CLI more intuitive.
Overall, your main logic, control flow, and error handling are solid; refining these messages will make the user experience more polished and professional.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| } | ||
|
|
||
| if (!source && !destination) { | ||
| console.error('Zero argument is provided'); |
There was a problem hiding this comment.
For the case when both source and destination are missing, the message "Zero argument is provided" is not very clear or user-focused and doesn't reflect that two arguments (source and destination) are required. Consider changing this to something like “Both source and destination paths are required” so users immediately understand what they must provide.
| } | ||
|
|
||
| if (!source || !destination) { | ||
| console.error('Only one argument is provided'); |
There was a problem hiding this comment.
Similarly, this message "Only one argument is provided" doesn’t explain that the program requires two paths (source and destination). Update it to a clearer instruction indicating that both paths must be provided, so the behavior matches the requirement and tests that expect such wording.
2 participants
No description provided.