add solution#373
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because the current implementation does not fully satisfy several core functional requirements around destination path handling, which are critical for correctness. Specifically, destinations that end with / (e.g. dir/) are not always treated as “must be an existing directory”, so commands like node index file.txt dir/ do not reliably throw an error when dir is missing as the specification requires. On src/app.js, the logic that computes filePath assumes the destination is always a directory, instead of first interpreting the destination as:
- (a) trailing slash → must be an existing directory, or error,
- (b) existing directory without trailing slash → move into that directory, or
- (c) non-existing path without trailing slash → treat as a new file name (rename).
Changes needed:
- At the argument handling / destination-parsing point in
src/app.js, add logic to interpretdestinationby checkingdestination.endsWith('/')(orpath.sep) and verify that such a path exists and is a directory; if it does not exist, explicitly throw/exit with an error instead of falling back tofs.rename. - When
destinationdoes not end with/, usefs.statto check if it exists and is a directory; if so, buildtarget = path.join(destination, path.basename(source)), otherwise treat it as the new filename andfs.rename(source, destination). - Replace the manual
readFile→writeFile→unlinkbranch with a singlefs.rename(source, computedTarget)once you’ve determined the correcttarget, and remove or narrow the outertry/catchso that missing-directory situations are not silently swallowed but instead produce the required error behavior.
As minor improvements, consider using a non-zero exit code for argument or directory errors and providing a clearer message like Destination directory does not exist. Your current use of fs APIs and basic rename/move behavior is a good foundation; once you tighten the destination interpretation and error handling, your solution will align well with the assignment’s mv-like semantics.
✨ 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 (process.argv.length <= 2) { | ||
| console.error('Zero argument is provided'); | ||
| process.exit(0); | ||
| } | ||
|
|
||
| if (process.argv.length === 3) { | ||
| console.error('Only one argument is provided'); | ||
| process.exit(0); | ||
| } |
There was a problem hiding this comment.
The current argument validation only checks the number of arguments, but the assignment also requires specific behavior based on the form of the destination (e.g. ending with /). You’ll need logic here (or shortly after) to interpret the destination string (trailing slash or not) and enforce directory existence rules accordingly.
| process.exit(0); | ||
| } | ||
|
|
||
| const filePath = path.join(destination, path.basename(sourceFile)); |
There was a problem hiding this comment.
filePath is computed as if destination is always a directory, but the requirements say that sometimes destination is a file name (rename case). You should compute the final target path conditionally based on whether destination is a directory (existing or indicated by trailing /) or a new file name.
| try { | ||
| const stats = await fs.stat(to); | ||
|
|
||
| isDirectory = stats.isDirectory(); | ||
| } catch { | ||
| isDirectory = false; | ||
| } |
There was a problem hiding this comment.
Using fs.stat(to) only checks whether the raw destination path exists as a directory, but the requirements say that a trailing / must be treated as a directory, and if that directory doesn’t exist you must throw an error. Right now, if to ends with / and doesn’t exist, you fall back to renaming instead of treating this as a “missing directory” error.
| if (isDirectory) { | ||
| await fs.writeFile(fullPath, sourceContent); | ||
| await fs.unlink(file); |
There was a problem hiding this comment.
In the isDirectory branch you manually read, write, and then unlink the file. This doesn’t incorporate the rule that dir/ must be considered a directory and must exist; also, you could use fs.rename(fullPathSource, fullPathDestination) once you compute the correct destination instead of doing read/write/unlink.
| } catch (err) { | ||
| console.error(err); | ||
| } |
There was a problem hiding this comment.
The outer try/catch logs any error but then continues without enforcing the required behavior when a destination directory doesn’t exist. Instead of catching everything generically here, consider explicitly checking for non-existent directories and exiting with an error per the spec, rather than masking these cases inside a general error handler.
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 affect correct CLI behavior, even though most core mv-like semantics are implemented well. The most critical point is that all error paths currently call process.exit(0) (e.g., around lines 11, 16, 38, 42, 49, 65, 72 in src/app.js), which signals success instead of failure; this breaks scripts that rely on exit codes and is a functional requirement for a CLI tool. Another functional concern is the extra parent-directory existence check in the move logic (lines 52–66): real mv and the task description both rely on the OS to reject invalid paths, so this additional check can incorrectly block valid moves; after you decide whether to is a directory or filename, you should just call fs.rename(file, to) and handle its error.
A third issue is that inside move (around line 32) you're using the outer destination variable instead of the to parameter for endsWith checks; this couples the function to global state and can cause incorrect behavior if move is reused, so switch to to.endsWith('/') || to.endsWith('\\'). These three items (exit codes, over-validation, and reliance on global destination) are functional, not cosmetic, and should be fixed before this behaves like a robust mv-style utility. As minor improvements, please also clean up error messages for clarity and grammar (e.g., use Source file does not exist and Destination directory does not exist) so users can quickly understand what went wrong. Once these changes are made, your solution will align well with the intended mv semantics and be solid for real-world CLI usage.
✨ 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
| isDirectory = false; | ||
| } | ||
|
|
||
| if (destination.endsWith('/') || destination.endsWith('\\')) { |
There was a problem hiding this comment.
Inside move, you’re checking destination.endsWith(...), which relies on the outer-scope variable instead of the to parameter passed into the function. This makes move depend on global state; consider using to.endsWith('/') || to.endsWith('\\') so the function is self-contained and easier to reason about.
| const parentDirectory = path.dirname(to); | ||
| let isParentDirectoryExists = false; | ||
|
|
||
| try { | ||
| const stats = await fs.stat(parentDirectory); | ||
|
|
||
| isParentDirectoryExists = stats.isDirectory(); | ||
| } catch { | ||
| isParentDirectoryExists = false; | ||
| } | ||
|
|
||
| if (!isParentDirectoryExists) { |
There was a problem hiding this comment.
The extra parentDirectory existence check isn’t required by the spec and can block valid paths that the real mv would simply attempt and let the OS reject. After interpreting whether to is a directory or a filename, you can just call fs.rename(file, to) and rely on its error if the parent directory doesn’t exist.
| console.error('Zero argument is provided'); | ||
| process.exit(0); | ||
| } | ||
|
|
||
| if (process.argv.length === 3) { | ||
| console.error('Only one argument is provided'); | ||
| process.exit(0); | ||
| } | ||
|
|
||
| const filePath = path.join(destination, path.basename(sourceFile)); | ||
|
|
||
| async function move(file, to, fullPath) { | ||
| let isDirectory = false; | ||
|
|
||
| try { | ||
| const stats = await fs.stat(to); | ||
|
|
||
| isDirectory = stats.isDirectory(); | ||
| } catch { | ||
| isDirectory = false; | ||
| } | ||
|
|
||
| if (destination.endsWith('/') || destination.endsWith('\\')) { | ||
| try { | ||
| if (isDirectory) { | ||
| await fs.rename(file, fullPath); | ||
| } else { | ||
| console.error('directory is not exist'); | ||
| process.exit(0); | ||
| } | ||
| } catch (err) { | ||
| console.error('Something went wrong'); | ||
| process.exit(0); | ||
| } | ||
| } else if (isDirectory) { | ||
| try { | ||
| await fs.rename(file, fullPath); | ||
| } catch { | ||
| console.error('directory does not exist'); | ||
| process.exit(0); | ||
| } | ||
| } else { | ||
| const parentDirectory = path.dirname(to); | ||
| let isParentDirectoryExists = false; | ||
|
|
||
| try { | ||
| const stats = await fs.stat(parentDirectory); | ||
|
|
||
| isParentDirectoryExists = stats.isDirectory(); | ||
| } catch { | ||
| isParentDirectoryExists = false; | ||
| } | ||
|
|
||
| if (!isParentDirectoryExists) { | ||
| console.error('Parent directory is no exist'); | ||
| process.exit(0); | ||
| } | ||
|
|
||
| try { | ||
| await fs.rename(file, to); | ||
| } catch { | ||
| console.error('Source file is not exist'); | ||
| process.exit(0); |
There was a problem hiding this comment.
All error cases currently process.exit(0), which signals success. For a CLI-like utility, it’s better to exit with a non-zero code (e.g. 1) on argument or filesystem errors so that scripts can detect failures.
| console.error('directory is not exist'); | ||
| process.exit(0); | ||
| } | ||
| } catch (err) { | ||
| console.error('Something went wrong'); | ||
| process.exit(0); | ||
| } | ||
| } else if (isDirectory) { | ||
| try { | ||
| await fs.rename(file, fullPath); | ||
| } catch { | ||
| console.error('directory does not exist'); | ||
| process.exit(0); | ||
| } | ||
| } else { | ||
| const parentDirectory = path.dirname(to); | ||
| let isParentDirectoryExists = false; | ||
|
|
||
| try { | ||
| const stats = await fs.stat(parentDirectory); | ||
|
|
||
| isParentDirectoryExists = stats.isDirectory(); | ||
| } catch { | ||
| isParentDirectoryExists = false; | ||
| } | ||
|
|
||
| if (!isParentDirectoryExists) { | ||
| console.error('Parent directory is no exist'); | ||
| process.exit(0); | ||
| } | ||
|
|
||
| try { | ||
| await fs.rename(file, to); | ||
| } catch { | ||
| console.error('Source file is not exist'); |
There was a problem hiding this comment.
Several error messages here (directory is not exist, Parent directory is no exist, Source file is not exist) are grammatically incorrect and not very descriptive. Consider clearer messages like Destination directory does not exist or Source file does not exist to make failures easier to understand.
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 progressing while you refine the remaining details. You’ve done a solid job implementing the core mv-like behavior: handling trailing slashes, distinguishing between directory and file destinations, and relying on fs.rename so the main move/rename logic is on the right track. The main issues to fix next are your exit codes: all error paths in src/app.js (for example around lines 11, 16, 38, 42, 49, and 56) currently call process.exit(0), which signals success instead of failure—these should use a non-zero code like process.exit(1) so scripts can correctly detect errors. In addition, some error messages are ungrammatical or unclear; for instance, change directory is not exist to something like Destination directory does not exist and Source file is not exist to Source file does not exist, and keep this wording consistent wherever that situation occurs. As a minor usability improvement, your messages for invalid argument counts (when zero or only one argument is provided) could briefly hint at the expected usage (e.g., Usage: node index <source> <destination>). Overall, your logic is close to the requirements; polishing the exit codes and messages will make your CLI much more robust and user-friendly, and you’re clearly making good progress.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| if (process.argv.length <= 2) { | ||
| console.error('Zero argument is provided'); | ||
| process.exit(0); |
There was a problem hiding this comment.
Using process.exit(0) for an error case signals success to the shell. For invalid argument counts you should use a non-zero exit code (e.g. process.exit(1)) so scripts can detect the failure.
| if (process.argv.length === 3) { | ||
| console.error('Only one argument is provided'); | ||
| process.exit(0); |
There was a problem hiding this comment.
Same as above: this is an error path but exits with code 0. Update to a non-zero code to align with correct CLI behavior.
| if (isDirectory) { | ||
| await fs.rename(file, fullPath); | ||
| } else { | ||
| console.error('directory is not exist'); | ||
| process.exit(0); |
There was a problem hiding this comment.
The message directory is not exist is grammatically incorrect and unclear; per the requirements, prefer something like Destination directory does not exist, and use a non-zero exit code instead of process.exit(0) here.
| } catch (err) { | ||
| console.error('Something went wrong'); | ||
| process.exit(0); |
There was a problem hiding this comment.
For unexpected errors you should not exit with code 0; use a non-zero exit code or let the error propagate so the process fails properly.
| try { | ||
| await fs.rename(file, fullPath); | ||
| } catch { | ||
| console.error('directory does not exist'); | ||
| process.exit(0); |
There was a problem hiding this comment.
This branch reports directory does not exist and exits with code 0. To meet the requirements, keep the message clear (e.g. Destination directory does not exist) and use a failing exit code (non-zero).
| try { | ||
| await fs.rename(file, to); | ||
| } catch { | ||
| console.error('Source file is not exist'); | ||
| process.exit(0); |
There was a problem hiding this comment.
Source file is not exist is both grammatically incorrect and exits with code 0. Update the message to Source file does not exist (as suggested) and use a non-zero exit code for this error.
No description provided.