Solution#378
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there is still a critical mismatch with the task requirements around error handling. The specification explicitly says that if the destination directory does not exist, an error must be thrown, but in your current src/app.js you only log the error and return (e.g., around the branch where you check fs.existsSync(targetDir)), so the process does not actually throw. This breaks required behavior for inputs like node index file.txt ./nonExistingDir/test, where path.dirname(destination) is a non-existent directory, and the program should fail with a thrown error instead of just printing a message.
To fix this, replace the console.error(...) + return path in that missing-directory branch with something like throw new Error('Destination directory does not exist') (or equivalent), so that the program terminates with an error as the task describes. Once that change is in place, the rest of your mv-like behavior (handling of renames, directory destinations with trailing /, and ambiguous names like a) appears to meet the functional requirements. As a minor improvement (not blocking), you may also standardize your error handling so all fatal conditions throw errors rather than mixing logs and early returns. Overall, you’re very close—after adjusting the missing-directory case to throw, your solution should fully comply with the task’s rules.
✨ 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 (!fs.existsSync(targetDir)) { | ||
| // eslint-disable-next-line no-console | ||
| console.error('Error: Destination directory does not exist'); |
There was a problem hiding this comment.
When the destination directory does not exist, the requirements say an error must be thrown. Here you only log an error and exit, which means the program doesn’t actually throw. Consider throwing an Error instead of just logging.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your solution is approved because it correctly implements the required mv-like behavior for files and handles all the specified edge cases. The moveFiles function performs proper renames, moves files into existing directories (including when the destination ends with a /), and correctly distinguishes between treating the destination as a directory versus a new filename. When the destination directory does not exist, you now throw an error instead of just logging, and the entrypoint’s try/catch cleanly reports that error, which aligns with the requirement to throw on missing destination directories. You also respect the constraint of supporting only basic file moves without additional flags or options. Overall, the functionality meets the core requirements, and no critical or blocking issues remain.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
No description provided.