-
-
Notifications
You must be signed in to change notification settings - Fork 191
Glasgow | 25-ITP-SEP | Mohammed Abdoon | Sprint 2 | Sprint-2 #834
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
cjyuan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you check if any of this general feedback can help you further improve your code?
https://github.com/CodeYourFuture/Module-Data-Groups/blob/general-review-feedback/Sprint-2/feedback.md
Doing so can help reviewers speed up the review process. Thanks.
I improved the code according the general feedback. thank you. |
| let outputObject = Object.create(null); | ||
| let splitString = string.split(" "); | ||
|
|
||
| for ( let i = 0; i < splitString.length; i++) { | ||
| let word = splitString[i].toLowerCase(); | ||
| word = word.replace(/[.,!?]/g, ''); | ||
|
|
||
| if (word.length != 0) { | ||
| if (!outputObject[word]) | ||
| outputObject[word] = 1; | ||
| else | ||
| outputObject[word]++; | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your code work. The followings are some improvements you can consider:
If we specified the word separator as "one or more space characters" instead of a single space character, the code in the loop could have less empty strings to deal with.
We could also normalize string (the parameter) by applying the "conversion operations" on lines 35-36 to string before line 32 once. Doing so could simplify the code within the loop (and slightly improve the performance).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the function could be much better than it is right now, but unfortunately, due to lack of time, I didn’t work on improving it (and also couldn’t do the other stretch exercises for the same reason). I’m already running late.
But will try to make the improvements when I am able to.
Thank you so mcuh.
|
Changeds have been made to the code. Thank you. |
|
This branch now contains only 2 files related to "Alarm Clock" exercise. The Sprint-2 script I reviewed are "gone". You can use "git revert" to undo some the most recent commits to restore your Sprint-2 files. |
Ah sorry my fault. I reverted the last commits. Now the files should exist. |
cjyuan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good! Well done!
Learners, PR Template
Self checklist
Changelist
I've implemented the required functions, fixed and made implementation tests.
Questions
No questions so far, thank you