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]Keith Tan #180

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

keithtqs
Copy link

Made the Find function case insensitive, also updated user guide as well as test expected output

@NichPang1
Copy link

A good QoL enhancement

@ojayjae
Copy link

ojayjae commented Sep 13, 2018

Excellent function and neatly implemented!

@C-E-OOI
Copy link

C-E-OOI commented Sep 13, 2018

A function that can greatly improve users experience as it provides convenience. Good one!

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.

Remember to change all documentations, e.g. usage message for Find command, after modifying its functionality.

@@ -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.

Consider the situation where you need to retrieve the name in its original case, can you think of perhaps a better place to convert the cases?

||
|| 0 persons listed!
|| 1 persons listed!
Copy link

Choose a reason for hiding this comment

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

You can try to come up with more test cases, e.g. the keywords are in upper cases etc.

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