-
-
Notifications
You must be signed in to change notification settings - Fork 88
Birmingham | 2026-MAR-SDC | Joy Opachavalit | Sprint 3 | Implement shell tools #408
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 3 commits
1576d23
34fe1d0
daac5ec
7e70d1c
a3dc8ae
9f4f7aa
054db41
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,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); | ||
|
|
||
| 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++; | ||
| } else { | ||
| console.log(line); | ||
|
Member
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 three branches here look quite similar and repetitive. In general, if you have multiple similar branches, it's more clear to extract the differences into variables, and then run the same code, i.e. so you'd only have one call to This way it's easier for someone reading the code to see what's the same / different in each case, and also avoids the hazard that someone updates one of the branches but forgets to update the other ones.
Author
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 have refactored the code to reduce repetition. Now, the differences are handled using a prefix variable, and there is only one console.log statement. This makes the code cleaner and easier to maintain. |
||
| } | ||
| }); | ||
| } catch (err) { | ||
| console.error(`cat: ${file}: No such file or directory`); | ||
|
Member
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. What exit code will your program have if something went wrong? What exit code should it have?
Author
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 have added proper exit codes. The program now exits with process.exit(1) when an error occurs, ensuring it signals failure appropriately.
Member
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. Are you sure the problem here is that the file didn't exist? What would happen e.g. if you didn't have permission to read the file? In general specific error messages are good, but misleadingly specific error messages are a problem - if we're not sure what went wrong (or if we have more information about what went wrong), we should present that information, rather than guessing. And if we are guessing, we should make it clear we're not sure what the problem exactly was and that this is a guess.
Author
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 have improved the error handling to differentiate between file-not-found (ENOENT) and permission errors (EACCES). The error messages now provide more accurate information about what went wrong. |
||
| } | ||
| }); | ||
| } | ||
|
|
||
| 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(); | ||
| 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 | ||
| } | ||
| 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 | ||
| } else if (arg === '-a') { | ||
| options.all = true; | ||
| } else { | ||
| directories = [arg]; | ||
|
Member
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. This meets the requirements for the examples listed in the README.md, but feels like it risks being confusing to users. If someone specified
Author
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 have updated the code to handle multiple directories. The program now lists files for all specified directories, making it more intuitive for users. |
||
| } | ||
| }); | ||
|
|
||
| directories.forEach((directory) => { | ||
| listFiles(directory, options); | ||
| }); | ||
| } | ||
|
|
||
| main(); | ||
| 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); | ||
| } | ||
|
Comment on lines
+65
to
+73
Member
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. What happens if someone specifies more than one of these flags? What should happen? Given the test cases we gave you in the README file, it's ok if your implementation doesn't do the same thing as the real
Author
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 have updated the code to handle multiple flags. If multiple flags are specified, the program now displays all the requested results together, ensuring user input is respected.
Member
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. This fixed some problems, but introduced some new ones too :) What happens if you Also, what happens if you
Author
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. Thank you for pointing this out! Here's how the program currently behaves: If no flags are provided (e.g., wc /some/file), the program defaults to displaying all metrics (lines, words, and bytes) for the file. This behavior is consistent with the standard wc command. If multiple files are provided with a single flag (e.g., wc -l /some/file /some/other/file), the program outputs the line count for each file on separate lines. However, it does not provide a total count across all files, which I’ve made the updates to align with the standard wc behavior. |
||
| }); | ||
|
|
||
| if (files.length === 0) { | ||
| console.error('Usage: wc [-l | -w | -c] <file>...'); | ||
| process.exit(1); | ||
| } | ||
|
|
||
| files.forEach((file) => { | ||
| const filePath = path.resolve(file); | ||
|
Member
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. Again, why do you need the
Author
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 have removed the unnecessary use of path.resolve and now directly use the file path provided by the user. This simplifies the code without affecting functionality. |
||
| countFile(filePath, 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.
What is this line doing? What would break if you removed it and just used
fileinstead offilePath?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.
I have removed the unnecessary use of path.resolve and now directly use file. The code works as expected without it, simplifying the implementation.