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
76 changes: 76 additions & 0 deletions implement-shell-tools/cat/cat.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
#!/usr/bin/env node
const fs = require('fs');
const path = require('path');

//shared line counter across all files(matches cat -n)
let globalLineCounter = 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.

You have a variable called globalLineCounter declared outside of any function, which means it exists for the whole script. While this works for your use case, can you think of any potential issues if you wanted to reuse your printFile function in another context, or if you wanted to run multiple file prints in parallel? How might you limit the scope of this variable to avoid unexpected side effects?

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 'globalLineCounter' is declared at the top level, so it is shared across all function calls and files processed. This works for your current use case, but can you think of any situations where having a global variable like this might cause unexpected behavior, especially if you wanted to reuse your printFile function elsewhere or in a different context?

Comment on lines +5 to +6
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

For example, comments like '//shared line counter across all files(matches cat -n)' or '//default: just print the line' are just repeating what the code is already showing. When your code is clear and variable names are descriptive, you often don't need to add a comment that just says what the code does. Instead, try to use comments to explain why something is done a certain way, or to clarify something that might not be obvious to someone reading your code later. Can you think of situations where a comment would be more helpful if it explained the reasoning behind a choice, rather than just describing the action?

Comment on lines +5 to +6
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

For example, comments like '//shared line counter across all files(matches cat -n)', '//-b option: number non-empty lines', '//-n option: number all lines', and '//default: just print the line' are mostly repeating what the code is already expressing clearly. When your code is already straightforward, do you think these comments help someone reading your code, or could they make it harder to spot truly important notes? Sometimes, too many obvious comments can make it harder to notice the comments that really matter.


function printFile(filePath, options) {
try {
const content = fs.readFileSync(filePath, 'utf-8');
const lines = content.split('\n');

lines.forEach((line) => {
if(options.numberNonEmpty) {
//-b option: number non-empty lines
if(line.trim()) {
process.stdout.write(
`${String(globalLineCounter).padStart(6)}\t${line}\n`
);
globalLineCounter++;
} else {
process.stdout.write('\n');
}
} else if(options.numberAll) {
//-n option: number all lines
process.stdout.write(
`${String(globalLineCounter).padStart(6)}\t${line}\n`
);
globalLineCounter++;
} else {
//default: just print the line
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, you have a comment saying '//default: just print the line'. Since the code right below it is a single line that prints the line, this comment doesn't add much value. When your code is already clear and self-explanatory, do you think you need to add a comment like this? Sometimes, too many obvious comments can make it harder to spot the important ones.

process.stdout.write(line + '\n');
}
Comment on lines +17 to +33
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 that the code for printing lines (with or without numbers) is repeated in a few places inside your forEach loop. If you ever wanted to change how lines are printed, you'd have to update it in multiple spots. Can you think of a way to extract this logic into a helper function, so you only need to update it in one place if requirements change?

Comment on lines +17 to +33
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 that the code for printing lines (with or without numbers) is repeated in a few places inside your forEach loop. If you ever wanted to change how lines are printed, you'd have to update it in multiple spots. How could you group this logic into a helper function so that you only need to update it in one place if you want to change the output format?

});

} catch (error) {
console.error(`Error reading file ${filePath}: ${error.message}`);
}
}

function main() {
const args = process.argv.slice(2);
const options = {
numberNonEmpty: false,
numberAll: false,
};
const filePatterns = [];
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 filePatterns might suggest that it stores patterns (like wildcards), but in your code it actually just stores file names. Do you think a name like fileNames or files would make the code's intent clearer to someone reading it for the first time?

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 name 'filePatterns' might make a reader think it stores wildcard patterns (like '*.txt'), but it actually just stores file names. Would a name like 'fileNames' or 'files' be clearer for someone reading your code?

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 'filePatterns' is used to store file paths, not patterns (like globs or wildcards). Naming it 'filePatterns' could confuse someone reading the code into thinking it supports pattern matching. What name might better reflect what this variable actually contains?

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 'filePatterns' is used to store file names, not patterns (like wildcards or globs). Naming it 'filePatterns' might confuse someone reading the code into thinking it supports pattern matching. What name could you use that more accurately describes what this variable holds?

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 'filePatterns' is used to store file paths, not patterns (like globs or wildcards). Do you think 'filePaths' or 'files' might be a more accurate name for this variable, so that readers are not misled about its contents?


args.forEach((arg) => {
if(arg === '-n') {
options.numberAll = true;
} else if(arg === '-b') {
options.numberNonEmpty = true;
} else {
filePatterns.push(arg);
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 name 'filePatterns' might make a reader think it stores wildcard patterns (like '*.txt'), but it actually just stores file names. Would a name like 'fileNames' or 'files' be clearer for someone reading your code?

}
});
// -b takes precedence over -n
if(options.numberNonEmpty) {
options.numberAll = false;
}

if(filePatterns.length === 0) {
console.log("cat: missing file operand");
process.exit(1);
}

const files = filePatterns;

files.forEach((file) => {
const resolvedPath = path.resolve(process.cwd(), file);
printFile(resolvedPath, options);
});
}

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

function listDirectory(dirPath, showAll, onePerLine) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Your function 'listDirectory' takes a parameter called 'dirPath', but sometimes you might pass it a file name (if the user gives a file instead of a directory). Do you think this name could be misleading for someone reading your code? What name might better reflect what the function expects?

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 parameter 'dirPath' in listDirectory is sometimes used for files as well as directories. Does this name always accurately describe what is being passed, or could it be misleading if a file path is provided?

try {
const entries = fs.readdirSync(dirPath, { withFileTypes: true });
const filtered = entries.filter((entry) => showAll || !entry.name.startsWith('.'));

if(onePerLine) {
filtered.forEach(entry => console.log(entry.name));
} else {
const names = filtered.map(entry => entry.name);
console.log(names.join(' '));
}
} catch (error) {
console.error(`Error reading directory ${dirPath}: ${error.message}`);
}
}
function main() {
const args = process.argv.slice(2);
// Check for options
const showAll = args.includes('-a');
const onePerLine = args.includes('-1');
Comment on lines +22 to +24
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Comments like '// Check for options', '//remove options from args', '// If no directory is specified, list the current directory', '//If a directory is specified, list that directory', '//Print header if multiple directories are listed', and '//add a blank line between directory listings if there are multiple directories' are mostly repeating what the code is already saying. When your code is clear, do you think these comments help, or could they be skipped to keep your code cleaner? Try to save comments for things that aren't obvious from the code itself.

//remove options from args
const directories = args.filter(arg => arg !== '-a' && arg !== '-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 'directories' variable actually contains both files and directories, not just directories. This could be confusing for someone reading the code. What might be a more accurate name for this variable?

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 'directories' variable actually holds both files and directories, not just directories. This could be confusing for someone reading the code. Can you think of a name that better reflects that this array may contain both files and directories?

Comment on lines +22 to +26
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Comments like '// Check for options', '//remove options from args', or '//If a directory is specified, list that directory' are just restating what the code is doing. When your code is already clear, these comments don't add much value. Instead, you could focus on commenting only when something is not obvious, or when you want to explain why you made a certain decision. How might you decide when a comment is actually helpful for someone reading your code?


// If no directory is specified, list the current directory
if(directories.length === 0) {
listDirectory(process.cwd(), showAll, onePerLine);
return;
}
//If a directory is specified, list that directory
directories.forEach((arg, index) => {
try {
const stats = fs.statSync(arg);
if(stats.isDirectory()) {
//Print header if multiple directories are listed
if(directories.length > 1) console.log(`${arg}:`);

listDirectory(arg, showAll, onePerLine);
//add a blank line between directory listings if there are multiple directories
if(directories.length > 1 && index < directories.length - 1) console.log('');
} else{
console.log(arg);// single 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.

On line 45, the comment '// single file' is right above a line that just prints the file name. Since the code is already very clear, do you think this comment helps, or could it be left out to keep things tidy?

}
} catch (error) {
console.error(`Error accessing ${arg}: ${error.message}`);
}
});
}
main();
71 changes: 71 additions & 0 deletions implement-shell-tools/wc/wc.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
#!/usr/bin/env node
const fs = require('node:fs');
// Function to count lines, words, and bytes in a 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.

For example, comments like '// Function to count lines, words, and bytes in a file' or '//print counts in the format of wc according to options' are just describing what the function or code is already showing. When your function and variable names are clear, you usually don't need to add a comment that just repeats that information. Instead, try to use comments to explain why something is done, or to clarify complex logic. What kind of information do you think would be more useful in a comment for someone new to this code?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Comments like '// Function to count lines, words, and bytes in a file', '//print counts in the format of wc according to options', and '//if no specific count options are provided, print all counts' are mostly repeating what the function names and code already say. Do you think these comments help someone understand your code, or could they be skipped? Try to focus comments on things that aren't obvious from reading the code.

function countFileContent(content) {
const lines = content.split('\n').length; // Count lines by splitting on newline characters
const words = content.trim().split(/\s+/).filter(Boolean).length; // Split by whitespace and filter out empty strings
const bytes = Buffer.byteLength(content, 'utf8');
return { lines, words, bytes };
}

//print counts in the format of wc according to options
function printCounts(filePath, counts, options) {
const parts = [];
if(options.line) parts.push(counts.lines);
if(options.word) parts.push(counts.words);
if(options.byte) parts.push(counts.bytes);
//if no specific count options are provided, print all counts
if(!options.line && !options.word && !options.byte) {
//default is to print all counts
parts.push(counts.lines, counts.words, counts.bytes);
}
console.log(parts.join('\t'),filePath);
}

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

//Separate options from file paths
const files = [];
args.forEach((arg) => {
if(arg === '-l') options.line = true;
else if(arg === '-w') options.word = true;
else if(arg === '-c') options.byte = true;
else files.push(arg);
});

if(files.length === 0) {
console.error('No files specified');
process.exit(1);
}

let totalCounts = { lines: 0, words: 0, bytes: 0 };
const multipleFiles = files.length > 1;

files.forEach(file => {
try {
const content = fs.readFileSync(file, 'utf-8');
const counts = countFileContent(content);

// Sum counts for total if multiple files
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 55, you wrote '// Sum counts for total if multiple files'. The code right below it is just adding up the counts, which is quite clear from the variable names and context. Do you think this comment is necessary, or could the code be understood without it?

totalCounts.lines += counts.lines;
totalCounts.words += counts.words;
totalCounts.bytes += counts.bytes;

printCounts(file, counts, options);
} catch (error) {
console.error(`Error reading file ${file}: ${error.message}`);
}
});

// If multiple files, print total counts
if(multipleFiles) {
printCounts('total', totalCounts, options);
}
}
main();
Loading