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;

files.forEach((file) => {
const filePath = path.resolve(file);

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 the cat function, filePath is created as a separate variable and then only used immediately once in the readFileSync call (and not reused elsewhere, for example in the error message). When a temporary variable is only used once right after it’s declared, it can be a hint that you might not need to name it at all.

One thing to think about: would inlining path.resolve(file) directly into fs.readFileSync keep the code just as readable for you, while slightly reducing the amount of state you have to track in your head? Or do you feel that having filePath named explicitly helps you understand what’s going on? Getting into the habit of asking yourself this question for single-use temporaries can help you find a good balance between clarity and conciseness.

To "like" or "dislike" this comment, please follow this link


try {
const data = fs.readFileSync(filePath, 'utf8');
const lines = data.split('\n');
Comment on lines +9 to +14

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 the cat function you create a filePath variable and then use it only once immediately in fs.readFileSync(filePath, 'utf8'). Since your error message already uses the original file variable, this extra variable isn’t adding much clarity at the moment. How do you feel about whether the separate filePath name helps you understand the code better, or whether inlining path.resolve(file) directly into readFileSync would keep things just as readable while removing one moving part to think about?

To "like" or "dislike" this comment, please follow this link


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

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 the inner lines.forEach loop you have two branches (options.numberNonEmpty && line.trim() and options.numberLines) that both do exactly the same two operations: print the line with a number and then increment lineNumber.

When the same logic lives in multiple branches like this, it can be easy to accidentally change one branch and forget the other if you later tweak the formatting (for example, changing the delimiter between the number and the text, or adjusting how the number is displayed). That kind of small divergence can create subtle inconsistencies that are tricky to notice.

How might you restructure this part so that the decision of whether to number the line is separate from how you number a line, so the numbering behavior itself only lives in one place?

To "like" or "dislike" this comment, please follow this link

});
} 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
}
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 this line you explain that hidden files are being skipped unless -a is specified. Because the if condition (!options.all && file.name.startsWith('.')) already reads almost like that sentence in plain English, the code is fairly self-explanatory. Comments that just restate what the code does can become noise over time and make it harder to spot the comments that carry real intent or explain tricky behavior. How might you rely on clear naming and the if condition itself here, and reserve comments for why a decision was made or for non-obvious edge cases instead?

To "like" or "dislike" this comment, please follow this link

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 this line the comment // Skip hidden files unless -a is specified is explaining exactly what the if (!options.all && file.name.startsWith('.')) { return; } branch already makes quite clear to someone reading the code. When comments just restate the condition, they can become noise and may get out of date if the logic changes later.

You might ask yourself: if you renamed options.all to something like includeHidden (or similar), would the code be self-explanatory without the comment? If so, simplifying or improving the naming could remove the need for the comment entirely and keep the code easier to maintain.

To "like" or "dislike" this comment, please follow this link

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 this line, the inline comment // Skip hidden files unless -a is specified is describing exactly what the if condition already makes quite clear: it checks for names starting with . and returns early when options.all is false. When comments restate the code literally, they can become noise and may drift out of sync if the logic changes later.

How might you make the code expressive enough that this comment isn’t needed at all? For example, could naming options.all or extracting the condition into a well‑named helper make the intent obvious without a comment? Thinking this way helps you reserve comments for why something is done, rather than what this simple line already shows.

To "like" or "dislike" this comment, please follow this link

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 = ['.'];

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 name directories suggests that it will hold multiple directory paths, but the logic only ever keeps the last non-flag argument by assigning directories = [arg] each time. This means that if someone reads the code, they might reasonably assume multiple directories are supported when in reality only one is used.

When names imply different capabilities than what the code actually does, it can make it harder to extend or debug later because future changes may be based on incorrect assumptions. How might you rename the variable or adjust the logic so that the name clearly reflects that, as written, only a single directory is ever used?

To "like" or "dislike" this comment, please follow this link


args.forEach((arg) => {
if (arg === '-1') {
// -1 is the default behavior, so no action needed
Comment on lines +30 to +31

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Here, the comment // -1 is the default behavior, so no action needed is again repeating exactly what the if (arg === '-1') {} branch already expresses (an empty branch means “we don’t need to do anything”). Because the behavior is simple and encoded directly in the control flow, the comment doesn’t add much and could become misleading if the default behavior ever changes.

When you see yourself adding comments like this, it can help to ask: If I removed this comment, would the code still be understandable to someone familiar with JavaScript and CLI tools? If the answer is yes, it might be better to omit the comment and let the code speak for itself.

To "like" or "dislike" this comment, please follow this link

} else if (arg === '-a') {
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.

This comment explains that -1 is the default behavior, but the body of the if is intentionally empty, which already hints that no extra work is needed for this flag. When comments describe simple, obvious behavior like this, they can become outdated if the behavior changes later or just add visual clutter. Could you consider whether the code would still be clear if this comment were removed, or perhaps expressed through a small refactor (for example, documenting supported flags in a help/usage string) rather than inline?

To "like" or "dislike" this comment, please follow this link

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.

Here the comment // -1 is the default behavior, so no action needed is documenting that this if branch is intentionally empty. Because the code literally “does nothing” in that case, the behavior is already visible: the option is accepted but doesn’t change anything.

When you see yourself adding a comment to explain that a branch does nothing, it can be a good moment to consider whether the code itself could make that intent clear. For example, could a small refactor (like parsing options separately, or documenting supported flags in help text) make this comment unnecessary, while still making it obvious that -1 is just a no-op?

To "like" or "dislike" this comment, please follow this link

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.

In main, the variable name directories suggests that the program supports working with multiple directories at once, but the current argument parsing only ever keeps a single directory (it overwrites the array each time it sees a non-flag argument, rather than adding to it). Because of this, someone reading the code might reasonably expect ls file1 dir2 to iterate over both file1 and dir2, but in practice only the last one is used.

How might you rename this variable (or adjust how it’s used) so that another reader can immediately see whether multiple directories are really supported or not? Aligning the name with the actual behavior can make it easier for future you (or others) to understand the supported usage just from reading the code.

To "like" or "dislike" this comment, please follow this link


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,
};

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);
Comment on lines +55 to +57

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 the main function, filePath inside the files.forEach callback is computed and then used only once, immediately in the countFile call. Similar to the cat example, this is a temporary that’s created and consumed in one place without being reused.

You might ask yourself: would calling countFile(path.resolve(file), options) directly make this loop just as clear to read, while avoiding an extra name? Or does the explicit filePath variable make it easier for you to see what’s going on? Being intentional about these one-use temporaries can help keep your code a bit leaner without sacrificing readability.

To "like" or "dislike" this comment, please follow this link

Comment on lines +55 to +57

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, you compute const filePath = path.resolve(file); and then immediately pass filePath into countFile(filePath, options) without using it for anything else. Since countFile doesn’t need the original file string, you might ask yourself whether this temporary variable is pulling its weight. Would it still be clear to you if you called countFile(path.resolve(file), options) directly, or do you find the named filePath makes the intent noticeably easier to follow? Thinking about that trade-off can help you decide when an extra variable is worth keeping.

To "like" or "dislike" this comment, please follow this link

});
}

main();
Loading