Skip to content

task solution #324

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
IvanRyabukha wants to merge 3 commits into
base: master
Choose a base branch
from
Open

task solution #324

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
13234eb
task solution
IvanRyabukha Sep 22, 2025
05aab03
fix
IvanRyabukha Sep 23, 2025
be050e2
hotfix
IvanRyabukha Sep 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 65 additions & 1 deletion src/app.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,65 @@
// write code here
const fs = require('fs');
Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor Sep 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The task requires using the asynchronous fs.promises API for all file system operations. Please import it instead of the synchronous fs module. You can achieve this by changing this line to const fs = require('fs').promises;.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor Sep 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The task requires using the promise-based fs module. You should import it like this: const fs = require('fs').promises;. This will give you access to asynchronous methods that you can use with await.

const path = require('path');

function move() {
Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor Sep 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good start on the move function. However, the task requirements point towards an asynchronous implementation. You are expected to use promise-based methods like fs.promises.stat() and fs.promises.unlink().

Consider refactoring this function to be an async function. This will allow you to use await to handle asynchronous operations cleanly and is a best practice in Node.js to avoid blocking the event loop with synchronous I/O operations like fs.statSync or fs.renameSync.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor Sep 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To correctly use the fs.promises API, this function should be declared as async. This will allow you to use await with the promise-based methods, making the code non-blocking and easier to read. For example: async function move() { ... }.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor Sep 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you'll be using await with methods from fs.promises, this function needs to be declared as async. For example: async function move() { ... }.

if (process.argv.slice(2).length !== 2) {
// eslint-disable-next-line no-console
console.error(
'Wrong number of arguments is supplied, should pass 2 arguments',
);
Comment on lines +7 to +9
Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor Sep 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned in the previous review, the application must throw an error when something goes wrong, not just log to the console. This allows the calling code (or the Node.js runtime) to know that the operation failed. Please replace this with throw new Error('...');. This applies to all error handling in this file.


return;
}

const [sourceFile, destinationFile] = process.argv.slice(2);

if (!fs.existsSync(sourceFile)) {
Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor Sep 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a synchronous call. The requirements state that you must use the fs.promises API for file system operations. After importing fs.promises, you could check for file existence asynchronously. A common way is to try to access it and catch any error. For example: try { await fs.access(sourceFile); } catch { throw new Error('Non-existent source file'); }.

// eslint-disable-next-line no-console
console.error('Non-existent source file');
Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor Sep 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The task requires that an error must be thrown if the source file does not exist. Currently, you are logging a message to console.error and then returning from the function.

While this informs the user, it doesn't behave like a thrown error (e.g., it results in a successful process exit code 0). Please change this to throw new Error('Non-existent source file');. This applies to all the error conditions in your code.


return;
Comment on lines +16 to +20
Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor Sep 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The requirements state that your application must throw an error in failure cases, not just log to the console. This is crucial for error handling in larger applications and for testing. Please replace the console.error and return with throw new Error('Non-existent source file');. This change should be applied to all error checks in this file.

}

const dirName = path.dirname(destinationFile);

if (!fs.existsSync(dirName)) {
Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor Sep 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check for the destination directory has a logical flaw. If destinationFile is a path like 'non-existent-dir/', path.dirname('non-existent-dir/') resolves to '.' (the current directory), which almost always exists. This check will pass, but the fs.renameSync call later will fail with an unhandled error because the directory doesn't exist.

You need a more robust way to validate the destination path. If destinationFile ends with a path separator, you should check if that path itself exists and is a directory.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor Sep 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are two issues here. First, this is a synchronous call and should be replaced with an asynchronous one from fs.promises. Second, this logic is incorrect for destination paths that end with a separator, like nonexistent/. In that case, path.dirname returns . which always exists, and your check fails to catch that the nonexistent directory is missing. You'll need to handle this case specifically before attempting the move.

// eslint-disable-next-line no-console
console.error('Destination directory does not exist');

return;
}

const normalizedSource = path.resolve(sourceFile);
const normalizedDest = path.resolve(destinationFile);

if (normalizedSource === normalizedDest) {
return;
}

if (
fs.existsSync(normalizedDest) &&
fs.statSync(normalizedDest).isDirectory()
) {
const newFilePath = path.join(normalizedDest, path.basename(sourceFile));

fs.renameSync(sourceFile, newFilePath);
Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor Sep 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the most critical issue. The task requires the "move" operation to be implemented as a two-step process:

  1. Copy the source file to the destination using streams (fs.createReadStream piped to fs.createWriteStream).
  2. Delete the original source file using fs.promises.unlink() only after the copy is successful.

Using fs.renameSync does not meet these specific technical requirements. You'll need to replace all instances of fs.renameSync with the stream-based copying logic followed by unlinking the source file.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor Sep 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned in the previous review, using fs.renameSync is not permitted for this task. The goal is to implement the move operation manually by copying the file and then deleting the original.

You should implement this by:

  1. Creating a read stream for the source file (fs.createReadStream).
  2. Creating a write stream for the destination file (fs.createWriteStream).
  3. Using stream.pipeline to connect them, which handles the data transfer and error handling properly.
  4. After the pipeline successfully completes, deleting the source file using await fs.unlink(sourceFile).


return;
}

if (destinationFile.endsWith(path.sep)) {
const newFilePath = path.join(normalizedDest, path.basename(sourceFile));

fs.renameSync(sourceFile, newFilePath);

return;
}

fs.renameSync(sourceFile, normalizedDest);
Comment on lines +45 to +58
Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor Sep 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the most critical part of the feedback. The task explicitly forbids using fs.rename() or fs.renameSync. The 'move' operation must be implemented by:

  1. Creating a readable stream from the sourceFile using fs.createReadStream().
  2. Creating a writable stream to the new destination path using fs.createWriteStream().
  3. Piping the read stream to the write stream to copy the file content.
  4. Once the copy is complete (you can wrap the stream logic in a Promise), you must delete the original sourceFile using await fs.unlink(sourceFile).

Please replace all fs.renameSync calls with this stream-based copy-and-delete logic.

}

move();

module.exports = {
move,
};