London | 25-SDC-Nov | Jesus del Moral | Sprint 3| Implement Shell Tools #258
Conversation
delmorallopez
added
Needs Review
delmorallopez
force-pushed
the
Implement-Shell-Tools
branch
2 times, most recently
from
0e0d3f5 to
af93a31
Compare
LonMcGregor
added
the
Review in progress
LonMcGregor
left a comment
LonMcGregor
left a comment
There was a problem hiding this comment.
Good start on this task. In addition to the comments in the files, consider this: can you see any issues that might arise from the names you have given to your scripts?
| program | ||
| .name("count-containing-words") | ||
| .description("Counts words in a file that contain a particular character") | ||
| .option("-c, --char <char>", "The character to search for", "e") |
There was a problem hiding this comment.
Is there a reason you added this specific counting feature? I tested it and it does not seem to work right.
There was a problem hiding this comment.
You’re right, the counting feature isn’t part of cat and shouldn’t be included. I’ve removed it and focused on matching cat’s behaviour for the required flags (-n and -b) and file globbing, keeping the output identical to the real tool
| globalLineCounter++; | ||
| } else if (numberNonblank) { | ||
| if (line.trim().length > 0) { | ||
| console.log(`${String(globalLineCounter).padStart(6)}\t${line} [${countInLine}]`); |
There was a problem hiding this comment.
You are duplicating some rather complex print formatting. is there a way to reduce this?
There was a problem hiding this comment.
I extracted the line-number formatting into a helper function so the control flow only decides whether a line should be numbered, not how it’s printed. This removes duplication and makes the logic easier to follow.
| // If multiple files, show total | ||
| if (multipleFiles) { | ||
| let output = ""; | ||
| if (options.l) output += `${total.lines.toString().padStart(8)} `; |
There was a problem hiding this comment.
You are repeating yourself a bit on the output printing and formatting. Do you think you can improve that in any way?
There was a problem hiding this comment.
I have created one function that take (lines, words, bytes), takes options and returns the formatted string
And then reuse it for each file and the total line
LonMcGregor
added
Reviewed
|
I have changed the names of the scrips, they were the same names, they can get conflicts |
delmorallopez
force-pushed
the
Implement-Shell-Tools
branch
from
e617639 to
0bc2cfa
Compare
delmorallopez
added
the
Needs Review
|
great work on this task, those updates really improve the scripts |
LonMcGregor
added
Complete
2 participants
Include the command cat, ls, wc
The changes meet the requirements of the task
I have tested my changes
Contains exercises to help you with task is to re-implement shell tools you have used.
Each folder is named after a group of tools, and contains exercises to practice using them.