Solution#362
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there are a couple of critical behaviors that do not yet match the specification and will cause incorrect results in required scenarios. First, in src/app.js you destructure the CLI args into file and destination and immediately call path.resolve(destination), which strips any trailing slash; this means a destination like dir/ may be treated as a file path dir instead of “file inside directory dir/”, violating the requirement to interpret a trailing slash as an explicit directory target. To fix this, keep the raw destination argument (e.g. const [file, rawDestination] = ...) and check rawDestination.endsWith('/') (and on Windows also consider path.sep) before calling path.resolve, then enforce directory semantics when that trailing slash is present. Second, when the intended destination directory does not exist, your code currently prints an error via console.error(...) and returns; the task requires that an error be thrown in this case so callers/tests can detect the failure, for example throw new Error('Destination directory does not exist: ' + destinationDir). These two issues are functional blockers because they can create a file with the wrong name or silently fail instead of signaling an error, which breaks the required mv-like behavior. Once you preserve the trailing-slash information and switch the missing-directory case to throwing an error, the core behavior should align much more closely with the specification. Keep the rest of your logic (read, write, unlink) as is; it’s clear and on the right track—these edge-case fixes will make your solution robust and fully compliant.
✨ 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
| const fs = require('fs'); | ||
|
|
||
| const moveFile = () => { | ||
| const [file, destination] = process.argv.slice(2); |
There was a problem hiding this comment.
You destructure the CLI args here into file and destination, but you never keep the original destination string to check for a trailing slash. The spec requires treating a destination that ends with / as a directory. Store the raw argument (e.g. const [file, rawDestination] = ...) before calling path.resolve so you can detect rawDestination.endsWith('/'). See the description for this rule.
| } | ||
|
|
||
| const sourcePath = path.resolve(file); | ||
| const destinationPath = path.resolve(destination); |
There was a problem hiding this comment.
You call path.resolve(destination) which removes the trailing slash; because of that the code cannot tell whether the user intended a directory (e.g. dir/) and may create a file named dir instead of throwing when the directory is missing. Detect the trailing slash on the original arg before resolving and enforce directory-exists behavior when the trailing slash is present.
| if (!fs.existsSync(destinationDir)) { | ||
| console.error( | ||
| `Destination directory does not exist: ${destinationDir}`, | ||
| ); | ||
|
|
||
| return; |
There was a problem hiding this comment.
When the destination directory is missing you console.error(...) and return. The task requires that an error be thrown in this case. Replace the console.error + return with throwing an Error (for example: throw new Error('Destination directory does not exist: ' + destinationDir)) so callers/tests can detect the failure.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Changes are needed because a critical requirement about destination handling is not yet met and one of the provided tests still fails. In src/app.js, you call path.resolve(destination) before inspecting the raw CLI argument, which loses the original trailing separator information; you should instead keep something like const rawDestination = process.argv[3]; and check rawDestination.endsWith('/') || rawDestination.endsWith(path.sep) before resolving so dir/ (and Windows-style separators) reliably enforce directory semantics. Related to this, when a destination path with a trailing separator is meant to be a directory, you should construct the final path with the source basename (e.g. path.basename(sourcePath)) so dir/ becomes dir/<originalFileName> rather than inadvertently including source subdirectories. Another blocking issue is that when the intended destination directory does not exist you currently console.error(...) and return; the specification requires that you throw new Error('Destination directory does not exist: ' + destinationDir) (in each place you do this) so callers and tests can programmatically detect the failure. These two behaviors are the reason the test case for moving into a directory with a trailing slash (e.g. dir/) is failing and they directly violate the task’s functional requirements. Once you preserve and inspect the raw destination before resolving, support both '/' and path.sep, throw an Error instead of logging and returning when the destination directory is missing, and use path.basename(sourcePath) when writing into a directory, your implementation should satisfy the remaining requirements. Your core read → write → unlink flow is solid, so focusing on these edge-case behaviors will complete the task well.
✨ 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
| } | ||
|
|
||
| const sourcePath = path.resolve(file); | ||
| const destinationPath = path.resolve(destination); |
There was a problem hiding this comment.
You call path.resolve(destination) here before inspecting the raw CLI destination. The task requires preserving and checking the raw destination (e.g. rawDestination.endsWith(...)) before calling path.resolve so trailing-slash semantics are not lost. Capture the raw argument first and check it prior to resolving.
| } | ||
|
|
||
| try { | ||
| if (destination[destination.length - 1] === '/') { |
There was a problem hiding this comment.
This check only looks for '/'. On Windows the path separator can be \. Use the raw destination (e.g. rawDestination.endsWith('/') || rawDestination.endsWith(path.sep)) and perform this check before resolving the path so the trailing-separator enforces directory semantics as required.
| if (!fs.existsSync(destination)) { | ||
| console.error(`Destination directory does not exist: ${destination}`); | ||
|
|
||
| return; |
There was a problem hiding this comment.
When the intended destination directory doesn't exist you console.error(...) and return. The specification requires throwing an error here so callers/tests can detect failure (e.g. throw new Error('Destination directory does not exist: ' + destination)), not just logging and returning.
| if (!fs.existsSync(destinationDir)) { | ||
| console.error( | ||
| `Destination directory does not exist: ${destinationDir}`, | ||
| ); | ||
|
|
||
| return; |
There was a problem hiding this comment.
Same issue here: logging and returning when destinationDir does not exist. Per requirements you must throw an Error so callers/tests can observe the failure (replace console.error(...); return; with throw new Error(...)).
|
|
||
| return; | ||
| } else { | ||
| fs.writeFileSync(path.join(destination, file), data); |
There was a problem hiding this comment.
When writing the file into a directory you do path.join(destination, file). If the original file CLI arg contains path segments (e.g. src/file.txt) this will create nested directories inside the target. Consider using the source basename (e.g. path.basename(sourcePath)) so only the filename is used when moving into a directory.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your solution as an educational override so you can keep moving forward while you tighten a few important details in src/app.js.
You correctly implemented the core move behavior (read → write → unlink) and handled common scenarios like renaming a file and moving into an existing directory using path.basename(sourcePath) when treating the destination as a directory.
However, near the destination handling logic in src/app.js (around the first path.resolve call and the trailing-slash checks), you should first capture const rawDestination = process.argv[3];, inspect that raw value for a trailing separator, and only then call path.resolve, checking both rawDestination.endsWith('/') and rawDestination.endsWith(path.sep) so Windows-style paths are treated as directories too.
In the same area, when you treat the destination as a directory, make sure your fs.existsSync and other checks use the resolved path (e.g. destinationPath or destinationDir) rather than the raw string, so behavior is consistent for relative and absolute paths.
At the points where you currently do console.error(...) and return for a missing destination directory (in the blocks that run when the destination is interpreted as a directory), the requirement is to throw an Error instead, with the exact message format Destination directory does not exist: followed by the directory path variable (for example, destinationDir).
A minor improvement is to keep your directory vs file decision logic grouped and clearly commented, so it’s obvious when the code is treating the destination as a directory because of a trailing separator versus because fs.statSync reports a directory.
Overall, you’re very close: the main work left is to align the trailing-separator detection and error throwing with the spec, and once those are fixed your implementation will match the required behavior well.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| } | ||
|
|
||
| const sourcePath = path.resolve(file); | ||
| const destinationPath = path.resolve(destination); |
There was a problem hiding this comment.
You call path.resolve(destination) here before following the requirement to preserve and inspect the raw CLI argument for a trailing separator. Capture the raw argument first (e.g. const rawDestination = process.argv[3];) and check its trailing separator before resolving into destinationPath so resolving can't remove the trailing-separator information.
| } | ||
|
|
||
| try { | ||
| if (destination.endsWith('/')) { |
There was a problem hiding this comment.
This check only tests for a POSIX '/' trailing separator. Per the spec you must accept both '/' and the platform separator. Use something like rawDestination.endsWith('/') || rawDestination.endsWith(path.sep) and ensure you inspect the raw argument (not the resolved one) for that condition.
|
|
||
| try { | ||
| if (destination.endsWith('/')) { | ||
| if (!fs.existsSync(destination)) { |
There was a problem hiding this comment.
When treating the destination as a directory you call fs.existsSync(destination) on the raw string. After resolving paths you should perform existence checks on the resolved path (e.g. destinationPath) for consistency. Also see the next comment about error handling when the directory is missing.
| try { | ||
| if (destination.endsWith('/')) { | ||
| if (!fs.existsSync(destination)) { | ||
| console.error(`Destination directory does not exist: ${destination}`); |
There was a problem hiding this comment.
The requirements mandate that when the intended destination directory does not exist you must throw an Error with the message Destination directory does not exist: <destinationDir> instead of logging and returning. Replace this console.error + return with throw new Error('Destination directory does not exist: ' + destinationPath) (or appropriate directory variable).
| console.error( | ||
| `Destination directory does not exist: ${destinationDir}`, |
There was a problem hiding this comment.
Same problem here: replace the console.error(...) and return with throwing an Error: throw new Error('Destination directory does not exist: ' + destinationDir) so callers/tests can detect the failure programmatically.
No description provided.