London |26-JUL-SDC |Shaghayeghfar| Sprint 3 |Implement Shell Tools#593
London |26-JUL-SDC |Shaghayeghfar| Sprint 3 |Implement Shell Tools#593shaghayeghfar wants to merge 3 commits into
Conversation
shaghayeghfar
added
📅 Sprint 3
SlideGauge
left a comment
SlideGauge
left a comment
There was a problem hiding this comment.
Good job, I've got several notes from me, could you address them please?
| } else if (args[i] === "-a") { | ||
| showHidden = true; | ||
| } else { | ||
| path = args[i]; |
There was a problem hiding this comment.
When you run ls sample-files/*, the shell passes many paths as separate arguments. Each loop does path = args[i] - after the loop, how many of those paths are left? What does the real ls show for a glob like that, and how could you keep all of them rather than just one?
| // Run wc for every file | ||
|
|
||
| for (let file of files) { | ||
| countFile(file); |
There was a problem hiding this comment.
Try the real wc sample-files/* with several files — what appears on the last line that your output doesn't have yet? Where in this loop could you accumulate the counts to produce that?
|
|
||
| // Function to count one file | ||
| function countFile(fileName) { | ||
| const content = fs.readFileSync(fileName, "utf8"); |
There was a problem hiding this comment.
cat.js wraps its readFileSync in try/catch, but this one doesn't - and sample-files/* includes the dir subdirectory. What happens when readFileSync is handed a directory, and what does that do to the files listed after it?
| try { | ||
| let content = fs.readFileSync(fileName, "utf8"); | ||
|
|
||
| let lines = content.split("\n"); |
There was a problem hiding this comment.
If a file ends in \n, what's the last element of content.split("\n")? Combined with console.log adding its own newline (line 42), how does your output compare to cat sample-files/1.txt - same number of lines, or one extra?
| if (fs.statSync(path).isFile()) { | ||
| console.log(path); | ||
| } else { | ||
| let files = fs.readdirSync(path); |
There was a problem hiding this comment.
readdirSync returns entries in whatever order the filesystem gives. Does the real ls do the same, or does it guarantee a particular order? What one call would make yours match?
| let lineNumber = 1; | ||
|
|
||
| // Check for flags | ||
| if (args[0] === "-n") { |
There was a problem hiding this comment.
Flags are only checked at args[0]. That covers the required command lines - but ask yourself: what would happen with -n file1 file2, or a flag that isn't first?
| let lines = content.split("\n").length - 1; | ||
|
|
||
| let words = content | ||
| .trim() |
There was a problem hiding this comment.
Nice — trim().split(/\s+/).filter(...) correctly returns 0 words for an empty file, an edge case that trips a lot of people up.
SlideGauge
added
Reviewed
|
Dear @SlideGauge Thank you very much for all your help, feedback, and the time you have taken to review my work. I really appreciate it. Thank you again for your support and guidance! |
shaghayeghfar
added
Needs Review
|
Good job, review complete! |
SlideGauge
added
Complete
2 participants
Learners, PR Template
Self checklist
Changelist
Implemented simplified versions of common Unix shell commands using JavaScript and Node.js