Skip to content
This repository has been archived by the owner on May 23, 2018. It is now read-only.

Find People #2

Merged
merged 17 commits into from
Nov 2, 2017
Merged

Find People #2

merged 17 commits into from
Nov 2, 2017

Conversation

hlmaab
Copy link
Collaborator

@hlmaab hlmaab commented Oct 28, 2017

SearchPeople(string) in URLConnectionReader class takes in a string and returns a list of people found. People and PeopleList classes store the information of found people.

3 Cases:
Found
Not found
Too many results

@hlmaab hlmaab closed this Oct 28, 2017
@hlmaab hlmaab reopened this Oct 28, 2017
@hlmaab hlmaab requested review from IniZio and removed request for patrick330602 October 29, 2017 09:40
@hlmaab hlmaab changed the title People Find People Oct 29, 2017
@IniZio
Copy link
Owner

IniZio commented Oct 31, 2017

Might add test cases in Google Drive, as mentioned in milestone 2 assignment on canvas?

results.append("Search Result(s):");

if (resultList ==null) {
results.append("\nNot found.");
Copy link
Owner

@IniZio IniZio Nov 1, 2017

Choose a reason for hiding this comment

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

might want to reply and return here? when i enter an impossible input e.g. 123, it will give reply:

Search Result(s):
Not found. Too many results...

replyToken,
reply
);
number=1; // get input and search for name
Copy link
Owner

Choose a reason for hiding this comment

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

might want to use a final variable to many it more understandable for later development?
e.g. number = PEOPLE_ITSC_INPUT
?

Copy link
Owner

Choose a reason for hiding this comment

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

Since other branches also use similar mechanism, will ignore this suggestion first and solve at once later

Copy link
Owner

@IniZio IniZio left a comment

Choose a reason for hiding this comment

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

Suggestions

  • Reply rather than just append when no results to prevent unexpected reply in later statements
  • use constant to represent later stage of reply

@hlmaab hlmaab mentioned this pull request Nov 2, 2017
@IniZio IniZio merged commit 7495b59 into master Nov 2, 2017
@IniZio IniZio deleted the people branch November 2, 2017 08:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants