London | 26-SDC-Mar | Khilola Rustamova| Sprint 3 |implementing shell tools #485
London | 26-SDC-Mar | Khilola Rustamova| Sprint 3 |implementing shell tools #485HilolaRustam wants to merge 2 commits intoCodeYourFuture:mainfrom
Conversation
HilolaRustam
added
📅 Sprint 3
illicitonion
left a comment
illicitonion
left a comment
There was a problem hiding this comment.
This is impressive (bash is hard!), but the exercise here was to implement the tools in javascript not in bash - can you take another look? :)
illicitonion
added
Reviewed
HilolaRustam
added
Needs Review
illicitonion
left a comment
illicitonion
left a comment
There was a problem hiding this comment.
This is generally looking good, but a few things to look at :) Well done!
| let numberLines = false; | ||
| let numberNonEmpty = false; | ||
|
|
||
| if (args[0] === "-n") { |
There was a problem hiding this comment.
What would happen if someone passed both flags (e.g. node my-cat.js -b -n /some/file)? What do you think should happen?
| const lines = content.split(/\r?\n/); | ||
|
|
||
|
|
||
| if (numberLines){ |
There was a problem hiding this comment.
This code is a bit repetitive - in both the numberLines case and the numberNonEmpty case you have the same code adding a prefix and incrementing the count.
Can you think how to avoid this, so we can see more easily what's the same between each branch, and what the differences are?
| } else { | ||
| console.log(content); | ||
| } |
There was a problem hiding this comment.
These lines aren't indented correctly
There was a problem hiding this comment.
When I ran ls -1 -a sample-files vs node my-ls.js -1 -a sample-files I saw slightly different output - can you check what's causing the difference?
| if (flag === "-l") { | ||
| console.log(`${lines} ${file}`); | ||
| } else if (flag === "-w") { | ||
| console.log(`${words} ${file}`); | ||
| } else if (flag === "-c") { | ||
| console.log(`${chars} ${file}`); | ||
| } else { | ||
| console.log(`${lines} ${words} ${chars} ${file}`); | ||
| } |
There was a problem hiding this comment.
This works if you specify exactly one flag, but the real wc supports multiple flags at the same time, e.g. wc -w -l /some/file - can you support this too?
| } | ||
|
|
||
| if (files.length > 1) { | ||
| if (flag === "-l") { |
There was a problem hiding this comment.
This is quite repetitive with the per-file printing - can you avoid that duplication?
illicitonion
added
Reviewed
2 participants
Learners, PR Template
Self checklist
Changelist
Implementing shell tools for cat, ls and wc is complete.
Questions