-
Notifications
You must be signed in to change notification settings - Fork 104
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
[W5.11][F10-2]Ooi Xi Yi #185
base: master
Are you sure you want to change the base?
Conversation
Amazing function!! |
A necessary edit to the function!! |
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.
Also consider the case that the keywords are not in lower cases, will this implementation still work? Try to add more test cases for different scenarios and see what happens.
@@ -19,7 +19,7 @@ public CommandResult execute() { | |||
AddCommand.MESSAGE_USAGE | |||
+ "\n" + DeleteCommand.MESSAGE_USAGE | |||
+ "\n" + ClearCommand.MESSAGE_USAGE | |||
+ "\n" + FindCommand.MESSAGE_USAGE | |||
+ "\n" + FindCommand.MESSAGE_USAGE.toLowerCase() |
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.
why do you have to change this message to lower case as well?
@@ -40,7 +40,7 @@ public static boolean isValidName(String test) { | |||
* Retrieves a listing of every word in the name, in order. | |||
*/ | |||
public List<String> getWordsInName() { | |||
return Arrays.asList(fullName.split("\\s+")); | |||
return Arrays.asList(fullName.toLowerCase().split("\\s+")); |
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.
Instead of converting everything to lower case in this method itself, can you find another place that's better to do the conversion?(what if you need to get the words in its original cases later on?)
Find command is no longer case sensitive