West Midlands | SDC-July | Gabriel Deng | Sprint 3 | Implement shell tools #129
Conversation
gai93003
added
the
Needs Review
LonMcGregor
added
Review in progress
LonMcGregor
left a comment
LonMcGregor
left a comment
There was a problem hiding this comment.
Great work implementing these tasks. I have some small comments on ways you could improve them further.
| for (const line of lines) { | ||
| if (options.b) { | ||
| if (line.trim() !== "") { | ||
| console.log(`${(lineNum).toString().padStart(6)} ${line}`); |
There was a problem hiding this comment.
Here is looks like there is some repetition of code across your different output options. Can you think of a way to clean this up?
| const filtered = content.filter((word) => word.charAt(0) !== '.') | ||
|
|
||
| if (opts.one) { | ||
| console.log(filtered.join('\n')) |
There was a problem hiding this comment.
What happens if you run the original ls command with both -a1 options? How does this compare to your implementation?
| totalChars += chars; | ||
|
|
||
| let output = ''; | ||
| if (opts.l) output += `${lines} `; |
There was a problem hiding this comment.
There is quite a bit of duplication in your printing here and where you do total output. Can you think of a way to make this code cleaner?
LonMcGregor
added
Reviewed
gai93003
added
the
Complete
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). If this PR needs reviewed, please add the 'Needs Review' label to this PR after you have resolved the issues listed above. |
2 participants
Learners, PR Template
Self checklist
Changelist
Briefly explain your PR.
Questions
Ask any questions you have for your reviewer.