-
-
Notifications
You must be signed in to change notification settings - Fork 13
London | Ameneh Keshavarz | Implement-shell-tools| WEEK3 #49
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?
London | Ameneh Keshavarz | Implement-shell-tools| WEEK3 #49
Conversation
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.
Thank you for your PR! I'm Borja, a volunteer, and I'll be reviewing your code.
The implementations of cat
, ls
and wc
are good! I have left a few edge cases that are not covered, as well as a few suggestions, but you're definitely on the right track :)
Besides that, would you mind including the package.json
and package-lock.json
files you used during development? For a stretch goal, could you tell me why we should or shouldn't commit them to a repository?
Keep up the good work!
implement-shell-tools/cat/cat.js
Outdated
//const nOption = program.opts().number; | ||
//const bOption = program.opts().number2; |
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.
Do we need to keep these lines around? They seem to be doing the same thing as the next line.
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.
Thanks, I removed the unnecessary commnets.
implement-shell-tools/cat/cat.js
Outdated
try { | ||
const content = await fs.readFile(path, { encoding: "utf-8" }); | ||
const lines = content.split("\n"); | ||
if(lines[lines.length - 1] === "") { |
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.
if(lines[lines.length - 1] === "") { | |
if (lines[lines.length - 1] === "") { |
In other parts of the code, there is a space between the if
and the opening parenthesis, so we should be consistent. This is purely a style comment, and doesn't affect the correctness of the program.
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.
Yes, I updated the code to be consistent.
implement-shell-tools/cat/cat.js
Outdated
if (nOption) { | ||
console.log(`${lineNumber++} ${line}`); | ||
} else if (bOption && line.trim()) { | ||
console.log(`${nonEmptyLineNumber++} ${line}`); |
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.
There seems to be something wrong with the output. When I run this code, the lines on multiple files are counted the with the same counter:
$ node cat.js -b sample-files/*.txt
1 Once upon a time...
2 There was a house made of gingerbread.
3 It looked delicious.
4 I was tempted to take a bite of it.
5 But this seemed like a bad idea...
6 There's more to come, though...
However, when we run cat, we can see that every line number is counted individually:
➜ cat -b sample-files/*.txt
1 Once upon a time... # From sample-files/1.txt
1 There was a house made of gingerbread. # From sample-files/2.txt
1 It looked delicious. # From sample-files/3.txt
2 I was tempted to take a bite of it.
3 But this seemed like a bad idea...
4 There's more to come, though...
How can we modify the code so that the lines are counted correctly?
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.
Thanks for the feedback. I checked my code, and the issue was that I expected line numbers to reset for each file. But the real cat -b also keeps numbering across multiple files without resetting. I've updated my script, and it now matches that behavior.
implement-shell-tools/cat/cat.js
Outdated
function printLinesWithOptions(lines) { | ||
lines.forEach((line, index) => { | ||
if (nOption) { | ||
console.log(`${lineNumber++} ${line}`); |
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 happens when lineNumber
goes into double-digits? In cat
, it keeps alignment:
$ cat very_long_file
...
10 .parse(process.argv);
...
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.
Yes, line numbers were misaligned. Fixed it using padStart() to align them like cat -n.
implement-shell-tools/cat/cat.js
Outdated
try { | ||
const content = await fs.readFile(path, { encoding: "utf-8" }); | ||
const lines = content.split("\n"); | ||
if(lines[lines.length - 1] === "") { |
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.
Why do we need this special case?
Furthermore, what happens if lines
is empty? I know that, because of how split
works, you'll always get at least one element, but as the code evolves, this may no longer be true.
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.
Thanks, the special case isn’t strictly necessary, but I included it to be safe in case the input source changes in the future. I’ll also make sure to handle potential empty results from split() to avoid issues as the code evolves.
implement-shell-tools/ls/ls.js
Outdated
if (onePerLine) { | ||
files.forEach(file => console.log(file)); | ||
} else { | ||
console.log(files.join(' ')); |
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.
In ls, files are printed with a certain amount of padding, so that if we have many files, they appear to be in a table:
➜ /bin/ls
ls.js package-lock.json README.md
node_modules package.json sample-files
This is not part of the assignment brief but, as a streacth goal, would you like to consider how to pad the outputs like that?
|
||
program | ||
.description('Count lines, words, and characters in the specified files') | ||
.option('-l, --lines', 'Count the lines') |
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.
When counting lines, your program counts one more than wc
:
➜ wc -l sample-files/3.txt
5 sample-files/3.txt
➜ node wc.js -l sample-files/3.txt
6 /Users/blorente/code/github.com/CodeYourFuture/Module-Tools/implement-shell-tools/wc/sample-files/3.txt
Why does this happen? How could we fix it so that we print the right number of lines?
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 was using data.split('\n').length, which overcounts lines. To match the behavior of wc -l, I updated it to:
const lines = (data.match(/\n/g) || []).length;
implement-shell-tools/wc/wc.js
Outdated
const lines = data.split('\n').length; | ||
const words = data.split(/\s+/).filter(Boolean).length; |
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.
This section of the code is processing data
twice: Once to split it into lines, and another one to split it into words. While this is correct, in very large files, this could potentially be quite slow.
Is there a way we could avoid processing the entirety of data
twice?
This is a stretch goal, as the current program is already correct.
implement-shell-tools/wc/wc.js
Outdated
if (options.lines !== false) totals.push(totalLines); | ||
if (options.words !== false) totals.push(totalWords); | ||
if (options.characters !== false) totals.push(totalCharacters); |
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.
Why did you add the !== false
clause to these lines? Is there any way we could express the same in a more concise way?
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.
The !== false checks were overly cautious, siince commander sets options to true or undefined, the concise and correct way is simply:
if (options.lines)
.option('-l, --lines', 'Count the lines') | ||
.option('-w, --words', 'Count the words') | ||
.option('-c, --characters', 'Count the characters') | ||
.parse(process.argv); |
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.
Your solution prints the full path of each file:
➜ node wc.js -l sample-files/3.txt
6 /Users/blorente/code/github.com/CodeYourFuture/Module-Tools/implement-shell-tools/wc/sample-files/3.txt
Whereas wc
only prints the path relative to the directory we run it from:
➜ wc -l sample-files/3.txt
5 sample-files/3.txt
How would you modify this code to print the relative path?
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 was printing the absolute path using path.resolve, which made the output differ from wc. I’ve updated the code to print the original file argument instead, so it now shows the relative path the we type.
Hi Borja, Thanks for the feedback and for reviewing my code! I looked at the edge cases you mentioned and add the package.json and package-lock.json files. We should commit both files because: package.json shows what packages the project needs. package-lock.json makes sure everyone installs the exact same versions. Thanks again |
Learners, PR Template
Self checklist
Changelist
Briefly explain your PR.
Questions
Ask any questions you have for your reviewer.