Skip to content

[W5.11][F09-2]Kang Ming#175

Open
kangmingtay wants to merge 4 commits into
nusCS2113-AY1819S1:masterfrom
kangmingtay:master
Open

[W5.11][F09-2]Kang Ming#175
kangmingtay wants to merge 4 commits into
nusCS2113-AY1819S1:masterfrom
kangmingtay:master

Conversation

@kangmingtay

Copy link
Copy Markdown

Ready for review: made the find function case-insensitive

@kianhong95

Copy link
Copy Markdown

Good feature to have in the event that user forgets the exact entry that was entered into the address book.

@kangmingtay kangmingtay changed the title [W5.11][F09-A]Kang Ming [W5.11][F09-2]Kang Ming Sep 13, 2018
@px1099

px1099 commented Sep 13, 2018

Copy link
Copy Markdown

Comments:

  • Nice improvement for the "find" command
  • Updated user guide

Possible improvements:

  • Add comments explaining the "makeNamesLowercase" method
  • Update test cases (expected.txt and FindCommandTest.java)

@stephlewyh stephlewyh left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hi Kang Ming, good job with the enhancement! note that you may also consider setting class attribute keywords to lowerKeywords, so that you can reuse the lowercase keywords for other purposes in your application.

import java.util.HashSet;
import java.util.List;
import java.util.Set;
import java.util.*;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The libraries you are using should be stated explictly.

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.

5 participants