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
Edit borrower parser and tests #171
Edit borrower parser and tests #171
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good except I think we need to deal with the commented out code. Are they there because we will possibly use them in the future?
@@ -18,7 +19,7 @@ public static void assertParseSuccess(Parser parser, String userInput, Command e | |||
try { | |||
Command command = parser.parse(userInput); | |||
assertEquals(expectedCommand, command); | |||
} catch (ParseException pe) { | |||
} catch (ParseException | CommandException pe) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we catch and deal with them separately?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup ok! It was auto-added by the IDE. Will change
@@ -31,7 +32,7 @@ public static void assertParseFailure(Parser parser, String userInput, String ex | |||
try { | |||
parser.parse(userInput); | |||
throw new AssertionError("The expected ParseException was not thrown."); | |||
} catch (ParseException pe) { | |||
} catch (ParseException | CommandException pe) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Likewise for here as well
Because we are not going to use Edit Command so I was thinking of safely deleting the whole thing later on in 1.4 or something when we fix bugs and stuff. Now I just comment out first so that it doesn't affect our current progress |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might want to check BorrowersRecord.java, we need to maintain both listOfBorrowers
and borrowersMap
, I think some of the methods we might have missed out on updating the borrowersMap
. Otherwise, good job!
*/ | ||
public void removeBorrower(Borrower borrower) { | ||
listOfBorrowers.remove(borrower); | ||
borrowersMap.remove(borrower); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the argument to be passed into borrowersMap.remove
is the BorrowerId since that is the key. So should it be borrowersMap.remove(borrower.getBorrowerId())
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay noted with thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Sorry for the confusion earlier haha
closes #137