Implement an IndexIdPair #206
Conversation
Tests are failing and have yet to be fixed
…into v1.3/Refactor
hopinxian
added
priority.High
| + "Example: " + COMMAND_WORD + " 1 " | ||
| + PREFIX_NAME + "National University of Singapore " | ||
| + PREFIX_ADDRESS + "21 Lower Kent Ridge Rd, Singapore 119077"; | ||
| + PREFIX_ADDRESS + "21 Lower Kent Ridge Rd, Singapore 119077";; |
There was a problem hiding this comment.
There might be a typo here (extra semi-colon)
| * @throws ParseException if unable to parse the Id or the index. | ||
| */ | ||
| public IndexIdPair(ArgumentMultimap argMultimap, Prefix prefix) throws ParseException { | ||
| requireAllNonNull(argMultimap, prefix); |
There was a problem hiding this comment.
As this is a public constructor, perhaps you could consider making this code more defensive by ensuring that Prefix could only be PREFIX_PERSON_ID or PREFIX_LOCATION_ID? Since a wrong argument is likely to originate from a command inputting the wrong prefix, an assert statement here should be good enough.
There was a problem hiding this comment.
Alternatively, checking and throwing an exception may make this constructor easier to test.
There was a problem hiding this comment.
Sure! I have added exception checking.
| } | ||
|
|
||
| @Test | ||
| public void constructor_validArgs_success() { |
There was a problem hiding this comment.
Might need to add the corresponding tests for constructor related to the comment in the IndexIdPair class.
| try { | ||
| assertEquals(new IndexIdPair(argMultimap, PREFIX_PERSON_ID), expectedPersonPair); | ||
| } catch (ParseException pe) { | ||
| throw new IllegalArgumentException("Invalid userInput.", pe); | ||
| } | ||
|
|
||
| // location index | ||
| argMultimap = ArgumentTokenizer.tokenize(indexString, PREFIX_LOCATION_ID); | ||
| IndexIdPair expectedLocationPair = new IndexIdPair(INDEX_FIRST, null, PREFIX_LOCATION_ID); | ||
| try { | ||
| assertEquals(new IndexIdPair(argMultimap, PREFIX_LOCATION_ID), expectedLocationPair); | ||
| } catch (ParseException pe) { | ||
| throw new IllegalArgumentException("Invalid userInput.", pe); | ||
| } |
There was a problem hiding this comment.
Are the try-catch statements required here as the test is not supposed to fail anyway, and the parse exception isn't part of an assertion.
There was a problem hiding this comment.
indexIdPair constructor might throw a ParseException. Hence, the solution is either to use a try catch block, or have the test method throw the parse exception. I chose to use try catch in this case.
There was a problem hiding this comment.
Ok, i went to look through other tests and it seems addressbook for person tests chooses to have the method throw exception. I will follow accordingly.
An IndexIdPair contains either an index or an Id which points to either a Person or a Location. The significance of implementing this pair is to make it scalable for all commands to be able to use either index or id to refer to a Person or a Location.
All commands possible have been updated to use this IndexIdPair where possible.
Close #197