Skip to content
Open
Show file tree
Hide file tree
Changes from all 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
60 changes: 60 additions & 0 deletions implement-shell-tools/cat/cat.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
#!/usr/bin/env node

const fs = require('fs');
const path = require('path');

function cat(files, options) {
let lineNumber = 1;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The variable 'lineNumber' is used to number lines, but its behavior changes depending on the options. For example, with '-b', it only numbers non-empty lines, but the name doesn't reflect this conditional behavior. When naming variables, it's helpful to consider if the name always matches its use, especially when options change how it's incremented.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The variable 'lineNumber' is used to number lines, but its behavior changes depending on the options. For example, with '-b', it only numbers non-empty lines, but the name doesn't reflect this conditional behavior. When naming variables, it's helpful to consider if the name always matches its use, especially when options change how it's incremented.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The variable 'lineNumber' is used to number lines, but its behavior changes depending on the options. For example, with '-b', it only numbers non-empty lines, but the name doesn't reflect this conditional behavior. When naming variables, it's helpful to consider if the name always matches its use, especially when options change how it's incremented.


files.forEach((file) => {
Comment on lines +6 to +9
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

In cat, the lineNumber variable is shared across all files in the files.forEach loop (it is declared once outside the loop and never reset). If someone reading this code assumed line numbers restarted for each file (which is how some cat implementations behave), the name lineNumber alone doesn’t make it obvious that it is actually a global counter across all files. This can be confusing when maintaining the code later. How might you rename this variable (or adjust where it is declared) so that it’s clearer whether you intend numbering to reset per file or to continue across files?

const filePath = path.resolve(file);

try {
const data = fs.readFileSync(filePath, 'utf8');
const lines = data.split('\n');

lines.forEach((line) => {
if (options.numberNonEmpty && line.trim()) {
console.log(`${lineNumber}\t${line}`);
lineNumber++;
} else if (options.numberLines) {
console.log(`${lineNumber}\t${line}`);
lineNumber++;
Comment on lines +17 to +22
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

In your cat function, the logic for printing lines with numbers (for both -n and -b options) is almost identical, except for the condition that triggers it. If you ever wanted to change how the line numbers are formatted, you'd have to update it in two places. Can you think of a way to extract the printing logic into a helper function so you only need to update it in one place?

} else {
console.log(line);
}
});
} catch (err) {
console.error(`cat: ${file}: No such file or directory`);
}
});
}

function main() {
const args = process.argv.slice(2);
const options = {
numberLines: false,
numberNonEmpty: false,
};

const files = [];

args.forEach((arg) => {
if (arg === '-n') {
options.numberLines = true;
} else if (arg === '-b') {
options.numberNonEmpty = true;
} else {
files.push(arg);
}
});

if (files.length === 0) {
console.error('Usage: node cat.js [-n | -b] <file>...');
process.exit(1);
}

cat(files, options);
}

main();
44 changes: 44 additions & 0 deletions implement-shell-tools/ls/ls.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
#!/usr/bin/env node

const fs = require('fs');
const path = require('path');

function listFiles(directory, options) {
try {
const files = fs.readdirSync(directory, { withFileTypes: true });

files.forEach((file) => {
if (!options.all && file.name.startsWith('.')) {
return; // Skip hidden files unless -a is specified
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

On line 12, there's a comment: // Skip hidden files unless -a is specified. The code right above it is a simple if-statement that checks if the file name starts with a dot and the -a option is not set, and then returns. For someone reading the code, it's quite clear that this line is skipping hidden files unless the all option is true. When code is self-explanatory, adding a comment can sometimes make the code harder to maintain, because if you change the code but forget to update the comment, it can become misleading. How might you decide when a comment is truly needed, and when the code speaks for itself?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

On line 12, there's a comment: // Skip hidden files unless -a is specified. The code right below it is a simple if-statement that checks if the file name starts with a dot and if the -a option is not set. For someone reading the code, the logic is quite clear from the condition itself. Sometimes, adding comments for very straightforward code can make the code look more cluttered and doesn't add much value. How do you decide when a comment is helpful versus when the code is already self-explanatory?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

On line 12, there's a comment: // Skip hidden files unless -a is specified. The code right below it is a simple if-statement that checks if the file name starts with a dot and if the -a option is not set. For someone reading the code, the logic is quite clear from the condition itself. Sometimes, adding comments for very straightforward code can make the code look more cluttered and doesn't add much value. How do you decide when a comment is helpful versus when the code is already self-explanatory?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

On line 12, there's a comment: // Skip hidden files unless -a is specified. The code right below it is a simple if-statement that checks if the file name starts with a dot and if the -a option is not set. For someone reading the code, the logic is quite clear from the condition itself. Sometimes, adding comments for very straightforward code can make the code look more cluttered and doesn't add much value. How do you decide when a comment is helpful versus when the code is already self-explanatory?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

On line 12, you have a comment: // Skip hidden files unless -a is specified. The code right above it is already very clear: it checks if the file name starts with a dot and if the -a option is not set, then it returns early. For simple and self-explanatory code like this, do you think the comment adds much value? Sometimes, too many obvious comments can make it harder to spot the comments that really matter. How might you decide when a comment is truly helpful versus when the code speaks for itself?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I've noticed you added a comment on line 12: // Skip hidden files unless -a is specified. Since the code right above it is already very clear (if (!options.all && file.name.startsWith('.')) { return; }), do you think the comment adds much value? Sometimes, if the code is self-explanatory, extra comments can make the code look cluttered. How might you decide when a comment is truly needed?

}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

On line 31, there's a comment: // -1 is the default behavior, so no action needed. Since the code doesn't do anything in this branch, and the comment just restates that, it might not be necessary. If the code is clear without the comment, do you think it's better to leave it out? What could you do if you wanted to make the code self-explanatory without needing this comment?

Comment on lines +11 to +13
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

On the return; line you’ve added the comment // Skip hidden files unless -a is specified. Because the condition if (!options.all && file.name.startsWith('.')) already expresses this behavior quite clearly, the comment doesn’t really add extra information beyond what the code says. Over time these kinds of comments can become a maintenance burden if the behavior changes but the comment doesn’t. How might you decide when the intent is obvious enough from the code that you can safely skip adding a comment?

console.log(file.name);
});
} catch (err) {
console.error(`ls: cannot access '${directory}': No such file or directory`);
}
}

function main() {
const args = process.argv.slice(2);
const options = {
all: false,
};

let directories = ['.'];

args.forEach((arg) => {
if (arg === '-1') {
// -1 is the default behavior, so no action needed
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

On line 31, there's a comment: // -1 is the default behavior, so no action needed. Since the code does nothing in this branch, and the option is not used elsewhere, the comment is not really necessary. If you find yourself writing a comment to explain why you're not doing anything, it might be a sign that the code could be written in a way that doesn't need that explanation. What do you think about removing this comment to keep the code clean and focused?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

On line 31, there's a comment: // -1 is the default behavior, so no action needed. Since the code doesn't do anything in this branch, and the comment just restates that, it might not be necessary. If the code is clear without the comment, do you think it's better to leave it out? What could you do if you wanted to make the code self-explanatory without needing this comment?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

On line 31, there's a comment: // -1 is the default behavior, so no action needed. Since the code doesn't do anything in this branch, and the comment just restates that, it might not be necessary. If the code is clear without the comment, do you think it's better to leave it out? What could you do if you wanted to make the code self-explanatory without needing this comment?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

On line 31, there's a comment: // -1 is the default behavior, so no action needed. Since the code doesn't do anything in this branch, and the comment just restates that, it might not be necessary. If the code is clear without the comment, do you think it's better to leave it out? What could you do if you wanted to make the code self-explanatory without needing this comment?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

On line 31, there's a comment: // -1 is the default behavior, so no action needed. Since the code does nothing in this branch, and the option is not used elsewhere, do you think this comment is needed? Sometimes, if the code is clear and the option is not handled, it might be better to leave it without a comment, or perhaps handle it in a way that doesn't require explanation. What do you think?

} else if (arg === '-a') {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

On line 31, there's a comment: // -1 is the default behavior, so no action needed. Since the code doesn't do anything in this branch, and the comment just restates that, it might not be necessary. If the code is clear without the comment, do you think it's better to leave it out? What could you do if you wanted to make the code self-explanatory without needing this comment?

Comment on lines +30 to +32
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The comment // -1 is the default behavior, so no action needed explains why the if (arg === '-1') branch is empty. Since the body does literally nothing, a future reader might already infer that -1 doesn’t change behavior. If you find yourself needing this comment, would it be clearer to express that default behavior in code instead (for example through naming or structure) rather than relying on a comment that might drift from the actual behavior later?

options.all = true;
} else {
directories = [arg];
}
});
Comment on lines +27 to +37
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The variable 'directories' is named as if it could hold multiple directories, but your code only ever allows one directory to be listed at a time. This could be confusing to someone reading your code, as they might expect it to handle multiple directories. How could you name this variable to better reflect its purpose and avoid confusion?

Comment on lines +27 to +37
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

In main, the directories variable is initialized as an array but later is overwritten completely when a non-flag argument is seen: directories = [arg];. This means only the last positional argument is ever used, even though the name directories (plural) and the array type both suggest multiple directories might be handled. That mismatch between the name and the actual behavior can make the code harder to reason about. If you wanted to keep only one directory, what name would better express that? Or, if you wanted to support multiple directories later, how could you change the logic to match the plural name?


directories.forEach((directory) => {
listFiles(directory, options);
});
}

main();
61 changes: 61 additions & 0 deletions implement-shell-tools/wc/wc.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
#!/usr/bin/env node

const fs = require('fs');
const path = require('path');

function countFile(filePath, options) {
try {
const data = fs.readFileSync(filePath, 'utf8');

const lines = data.split('\n').length;
const words = data.split(/\s+/).filter(Boolean).length;
const bytes = Buffer.byteLength(data, 'utf8');

if (options.lines) {
console.log(`${lines}\t${filePath}`);
} else if (options.words) {
console.log(`${words}\t${filePath}`);
} else if (options.bytes) {
console.log(`${bytes}\t${filePath}`);
} else {
console.log(`${lines}\t${words}\t${bytes}\t${filePath}`);
}
} catch (err) {
console.error(`wc: ${filePath}: No such file or directory`);
}
}

function main() {
const args = process.argv.slice(2);
const options = {
lines: false,
words: false,
bytes: false,
};

Comment on lines +28 to +35
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

In main, options is a single object that is shared for all files when you loop over files and call countFile(filePath, options). Given how you parse the flags, that means the same set of options is used for each file, and you don’t clear or change them between calls. If a future change ever modified options inside countFile, this shared scope could cause subtle bugs where one file’s processing affects another. Do you think passing a fresh options object per file (or ensuring countFile treats options as read-only) would make this safer and clearer?

const files = [];

args.forEach((arg) => {
if (arg === '-l') {
options.lines = true;
} else if (arg === '-w') {
options.words = true;
} else if (arg === '-c') {
options.bytes = true;
} else {
files.push(arg);
}
});

if (files.length === 0) {
console.error('Usage: wc [-l | -w | -c] <file>...');
process.exit(1);
}

files.forEach((file) => {
const filePath = path.resolve(file);
countFile(filePath, options);
});
}

main();
Loading