London | 25-SDC-Nov | Zohreh Kazemianpour | Sprint 3 | Implement Shell Tools-NodeJS #239
Conversation
zohrehKazemianpour
added
the
Needs Review
zohrehKazemianpour
force-pushed
the
shell-NodeJS
branch
from
cb635a6 to
b91a308
Compare
zohrehKazemianpour
changed the title
zohrehKazemianpour
changed the title
LonMcGregor
left a comment
LonMcGregor
left a comment
There was a problem hiding this comment.
Good work on these tasks, sorry for the delay in getting a review.
Good use of comments to explain throughout.
There are some places you could improve implementations further
implement-shell-tools/cat/cat.js
Outdated
| // Process each file | ||
| for (const path of program.args) { | ||
| const hasNumberFlag = program.opts().number; // True if user used -n flag | ||
| const hasBFlag = program.opts().numberNonBlank; |
There was a problem hiding this comment.
Do you think the name "hasBFlag" explains well what the variable is doing?
There was a problem hiding this comment.
Thanks for catching this. No, it's not descriptive at all. I needed to read the program carefully to fully understand what it was doing. I've changed it to shouldNumberNonBlank.
implement-shell-tools/cat/cat.js
Outdated
| return line; | ||
| } else { | ||
| lineNumber = lineNumber + 1; | ||
| return ` ${lineNumber} ${line}`; |
There was a problem hiding this comment.
There is some duplication here, can you see any way to make this code a bit more concise?
There was a problem hiding this comment.
That is right, the return statement is used twice. I used the ternary operator to remove the redundant ones.
| program | ||
| .name("ls") | ||
| .description("list directory contents") | ||
| .option("-1, --one", "Force output to be one entry per line") |
There was a problem hiding this comment.
You include the 1 option here, do you ever use it?
There was a problem hiding this comment.
Thanks for pointing that out. I realized the flag wasn't doing anything because my output was always vertical. I’ve refactored it to be horizontal by default, so the -1 flag now correctly 'forces' the single-column output.
There was a problem hiding this comment.
Hi @LonMcGregor ,
I hope you had a lovely holiday.
Thanks for your patience with this! It took me a bit longer to get back to these changes as I had to step away for a week to complete my English and Life in the UK tests, which was immediately followed by the Christmas break.
I've now addressed all the feedback.
Happy New Year to you, and I look forward to your thoughts on the updates!
LonMcGregor
added
Reviewed
zohrehKazemianpour
added
the
Needs Review
|
Good work on these updates, this task looks complete now! |
LonMcGregor
added
Complete
Thanks for the complete label! |
2 participants
Self checklist
This PR contains implementing shell tools (cat, ls, wc) in JavaScript with NodeJS using the commander library.