Skip to content
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] Ong Jia Jian #176

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ojayjae
Copy link

@ojayjae ojayjae commented Sep 13, 2018

Made 'find' case insensitive and updated expected output and user guide

Ready for Review!

@NichPang1
Copy link

Simple but very practical enhancement. Great job

@keithtqs
Copy link

A necessary improvement as users might not remember how the contact was saved in terms of case sensitivity

@C-E-OOI
Copy link

C-E-OOI commented Sep 13, 2018

An improvement that is very considerate of users. Nice!

Copy link

@fanyiii fanyiii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job updating user guide and test cases! Remember to also update the usage message for Find command and think of how to improve the implementation.

@@ -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+"));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the implication of converting all words to lower case here? (what if later on you need to retrieve the words in their original forms). Think of a better place to do the conversion, or perhaps add a new method.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants