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][W10-4]ONG YI CHONG #15

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@yichong96
Copy link

yichong96 commented Feb 9, 2019

Enhanced find command to also include person's whose keywords contain substrings in the address book names.

Will be adding unit tests in the future.

@dlqs

This comment has been minimized.

Copy link

dlqs commented Feb 10, 2019

Hi yi chong, everything looks good. As good practice, you want to consider keeping your commit titles under 50 characters and write in the imperative.

@yichong96

This comment has been minimized.

Copy link
Author

yichong96 commented Feb 11, 2019

Thanks Donald, appreciate your comments !

@kylase-learning
Copy link

kylase-learning left a comment

Great to see you using TDD workflow, you have also submitted your changes via your own branch and done the documentation and also added the new inputs and outputs for the new command. 👍

Please close this PR after reading comments. Thanks!

|| 1 persons listed!
|| ===================================================
|| Enter command: || [Command entered: find Char Betsy]
|| 1. Betsy Choo Tags: [secretive]

This comment has been minimized.

@kylase-learning

kylase-learning Feb 11, 2019

Shouldn't this return both Betsy Choo and Charlie Dickson?

This comment has been minimized.

@yichong96

yichong96 Feb 11, 2019

Author

I have coded the substring method in a way such that if both keywords are substrings of the names then Betsy and Dickson will both be returned. If either one is a complete name, then the substring method would return the person with the complete name only. I have updated this in my user guide and I hope it doesnt confuse people >.<

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