Skip to content
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
1 change: 1 addition & 0 deletions implement-shell-tools/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
.venv
48 changes: 48 additions & 0 deletions implement-shell-tools/cat/cat.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
import process from "node:process";
import { promises as fs } from "node:fs";

// THis will give an array without the path to node and to the file.
const argv = process.argv.slice(2);

//Get line numbers.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does this comment provide value beyond the variable names?

If yes, could you change the variable names to make this comment no longer necessary?

const showNumbers = argv.includes("-n");
const showNonBlankNumbers = argv.includes("-b");

//filter the - from the array argv as it's a flag.
const filePaths = argv.filter((arg) => !arg.startsWith("-"));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What would happen if someone passed a -q flag here? What should happen?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thanks for reviewing, you are right to point that out, my logic will let the user confused thinking that the -q flag exists and worked, I implemented in place a flag check and ensured print an error and exit with not 0 code.


const flagsUsed = argv.filter((arg) => arg.startsWith("-"));
const supportedFlags = ["-n", "-b"];
for (const flag of flagsUsed) {
if (!supportedFlags.includes(flag)) {
console.error(`Invalid option try 'node cat.js --help' for more info.`);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What does node cat.js --help do? As far as I can tell, it will probably print this error?

process.exit(1);
}
}

let counterLines = 1;

for (const path of filePaths) {
try {
const content = await fs.readFile(path, "utf-8");

//split the text at the new line character.
const splitLines = content.split("\n");

splitLines.forEach((line) => {
let prefix = "";

if (showNonBlankNumbers) {
if (line.trim() !== "") {
prefix = `${counterLines++} `;
}
} else if (showNumbers) {
prefix = `${counterLines++} `;
}
Comment on lines +31 to +37

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would think about collapsing this into one if:

if (showNumbers || (howNonBlankNumbers && line.trim() !== "")) {

because they have the same intent, but wouldn't push strongly for this if you prefer it as-is :)

console.log(`${prefix}${line}`);
});
} catch (error) {
console.error(`Could not read: ${path}`);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Right now you're always printing the same error message if something went wrong.

What are the advantages/disadvantages to including the specific error message in the output to the user?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

The advantages is that will show the users the error and allow him to adjusted themselves.
The disadvantages is that it may leak sensitive data like a name in the path.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah, this is generally correct, though specific on names in the path, the user presumably has access to the name in the path because if they didn't they wouldn't have been able to pass it as an argument.

process.exit(1);
}
}
32 changes: 32 additions & 0 deletions implement-shell-tools/ls/ls.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
import fs from "node:fs";
import process from "node:process";

// This will give an array without the path to node and to the file.
const argv = process.argv.slice(2);

// filter the flag to find the target folder.
const filePaths = argv.filter((arg) => !arg.startsWith("-"));
const showHiddenFiles = argv.includes("-a");

if (filePaths.length > 1) {
console.error("Error: This version only supports one directory path a time.");
process.exit(1);
}
// if no folder provide we use the current one
const target = filePaths[0] || ".";

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What would happen if someone specified multiple paths, e.g. ls /some/file /some/other/file? What does your implementation do?

The README.md only requires that your programme works with simple paths, but I would recommend implementing support for multiple. But if you don't implement that, you generally want to give an error to the user if they supply input you don't expect, rather than just ignoring it.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thanks for reviewing, I should at least display an stderr and exit .

// read the file.
const files = fs.readdirSync(target);

// save the result into the variable.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Almost all of your comments provide no value over just reading the code. Comments should explain why we're doing something (if it's not obvious), or explain something particularly confusing/tricky. If you can read the code, the comments here aren't useful - please consider all of them and remove any you think aren't useful.

let filteredFIles = files;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
let filteredFIles = files;
let filteredFiles = files;

if (!showHiddenFiles) {
filteredFIles = files.filter((file) => !file.startsWith("."));
} else {
// we use spread operator to merge the paths.
filteredFIles = [".", "..", ...files];
}

// Print using -1 .
filteredFIles.forEach((file) => {
console.log(file);
});
46 changes: 46 additions & 0 deletions implement-shell-tools/wc/wc.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
import process, { exit } from "node:process";
import fs from "node:fs";

const argv = process.argv.slice(2);

let showWords = argv.includes("-w");
let showLines = argv.includes("-l");
let showBytes = argv.includes("-c");
let showCharacters = argv.includes("-m");

// filter flags, and getting the string of filename
const filePaths = argv.filter((arg) => !arg.startsWith("-"));
// if no flags enable all.
if (!showLines && !showCharacters && !showWords && !showBytes) {
showLines = true;
showCharacters = true;
showWords = true;
showBytes = true;
}
// fix bug .length ===0 will be true if nothing provided, instead !filePath will return empty array which result true.
if (filePaths.length === 0) {
console.error("PLease provide a file path");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You frequently have typos where the second letter after a capital letter is also capitalised. Please look out for these and fix them.

process.exit(1);
}
// loop trough the array to get each file path.
filePaths.forEach((filePath) => {
const content = fs.readFileSync(filePath, "utf-8");

const lines = content.split("\n").length - 1;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why do you have to - 1 here?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thanks for reviewing, I used -1 because my vscode automatically add a new line when save, so in this case split method will add an empty element, however in other environments this logic my fail if no trialling new line added .

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

We may add a conditional check using .pop method in case of empty string at the end

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please do the conditional pop check :)

const words = content
.trim()
.split(/\s+/)
.filter((word) => word != "").length;
// here I used Buffer.byteLength even if characters and bytes can be the same number .length, however sometimes an emoji or special characters can be heavier 2b or 4b
const bytes = Buffer.byteLength(content);
const characters = content.length;

let output = "";

if (showLines) output += `${lines} `;
if (showWords) output += `${words} `;
if (showBytes) output += `${bytes} `;
if (showCharacters) output += `${characters} `;

console.log(`${output} ${filePath}`);
});

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Your output doesn't match real wc's output if multiple files were passed. Please make sure to run the examples given in the READMEs and that your behaviours match.

Loading