London | 26-SDC-July | Alex Jamshidi | Sprint 3 | Implement shell tools - #635
London | 26-SDC-July | Alex Jamshidi | Sprint 3 | Implement shell tools #635Alex-Jamshidi wants to merge 15 commits into
Conversation
Alex-Jamshidi
added
📅 Sprint 3
LonMcGregor
added
the
Review in progress
LonMcGregor
left a comment
LonMcGregor
left a comment
There was a problem hiding this comment.
Good start but there is quite a bit of work still to do.
You've built your own argument handling system. Is your way of handling options/arguments always going to be the best? Is there a better way of doing this?
| function bFlag() { | ||
| return allFilesContents.map((fileContent) => { | ||
| let lineNumber = 1; | ||
| return fileContent.map((line, index) => { |
There was a problem hiding this comment.
A map is typically used to change one array into another using a function that doesn't have side effects. Here, you are changing a lineNumber variable each time the map runs. This isn't wrong, but just be careful of side effects, especially if you ever want to run things asynchronously.
There was a problem hiding this comment.
I see - I have refactored my code, so now I use a foreach to mutate the original array rather than mapping a new one.
|
|
||
| function Flag1() { | ||
| if (printInList) { | ||
| outputString = outputString.replaceAll("\t", "\n"); |
There was a problem hiding this comment.
I like that you are using lots of task specific functions.
But, is using global variables in this way a safe way of programming this? Is there a way that would have the data flow through your app in a more semantic way?
There was a problem hiding this comment.
I have rewritten many of the functions to allow the data to flow through, rather than the functions independently manipulating data stored in global variables.
| } | ||
|
|
||
| function print() { | ||
| if (printInList == true) { |
There was a problem hiding this comment.
Think about how if conditions work, and check this line again. Are you writing code in the most optimal way?
| } | ||
|
|
||
| function deleteOutputs() { | ||
| if (!deleted) { |
There was a problem hiding this comment.
Can you explain your thinking that led to a design where you need to manually handle your resources like this?
There was a problem hiding this comment.
I think it was a little convoluted...
Given no flags displays all data, but the presence of any flags means only the flagged data is displayed.
I decided that if any flag was given: all data should be deleted and just the data for that flag added back in.
This would be the same for all flags.
I've decided to create a separate array which lists displayed metrics from flags to simplify this.
I've also refactored a lot.
LonMcGregor
added
Reviewed
…d through functions as arguments - also fixed an if boolean
By which I assume you mean using the commander library? Yes this would be simpler to write and an ultimately more effective way to handle things |
Alex-Jamshidi
added
Needs Review
2 participants
Learners, PR Template
Self checklist
Changelist
cat
Handles all commands correctly:
cat sample-files/1.txt
cat -n sample-files/1.txt
cat sample-files/.txt
cat -n sample-files/.txt
cat -b sample-files/3.txt
Stretch:
Handles multiple flags and incorrect flags
(although does not parse b and n correctly when used together)
wc
Handles all commands correctly:
wc sample-files/*
wc -l sample-files/3.txt
wc -w sample-files/3.txt
wc -c sample-files/3.txt
wc -l sample-files/*
wc -w -l sample-files/3.txt
wc -w -l sample-files/*
Stretch:
Handles multiple flags and incorrect flags
Handles flag order
ls
Handles all commands correctly:
ls sample-files
ls sample-files/1.txt
ls sample-files/
ls -1
ls -1 sample-files
ls -1 -a sample-files
Stretch
Handles multiple flags and incorrect flags
Handles flag order
Task ID: CYF-1150