-
Notifications
You must be signed in to change notification settings - Fork 2
implements-shell-tool #3
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? |
||
|
|
||
| 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 | ||
| process.stdout.write(line + '\n'); | ||
| } | ||
|
Comment on lines
+17
to
+33
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 = []; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? |
||
|
|
||
| args.forEach((arg) => { | ||
| if(arg === '-n') { | ||
| options.numberAll = true; | ||
| } else if(arg === '-b') { | ||
| options.numberNonEmpty = true; | ||
| } else { | ||
| filePatterns.push(arg); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||
| 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) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? |
||
| 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'); | ||
| //remove options from args | ||
| const directories = args.filter(arg => arg !== '-a' && arg !== '-1'); | ||
|
|
||
| // 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 | ||
| } | ||
| } catch (error) { | ||
| console.error(`Error accessing ${arg}: ${error.message}`); | ||
| } | ||
| }); | ||
| } | ||
| main(); | ||
| 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 | ||
| 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 | ||
| 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(); |
There was a problem hiding this comment.
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?