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
Info command #125
Info command #125
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.
Overall, the Info Command feature was well done. Good job!
@@ -5,18 +5,25 @@ | |||
*/ | |||
public class Messages { |
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.
Do we want to consider splitting the messages into different classes? For example, BookMessages, CommandMessages, etc.
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.
We can look into it! If i'm not wrong the other PRs have new Messages as well so we can split it once we merge everyone's PR
@@ -48,7 +50,7 @@ public void parse_missingParts_failure() { | |||
assertParseFailure(parser, VALID_TITLE_BOOK_1, MESSAGE_INVALID_FORMAT); | |||
|
|||
// no field specified | |||
assertParseFailure(parser, "1", EditCommand.MESSAGE_NOT_EDITED); | |||
assertParseFailure(parser, VALID_INDEX_ONE, EditCommand.MESSAGE_NOT_EDITED); |
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.
Good job replacing magic numbers with a symbolic constant!
@@ -11,11 +11,14 @@ | |||
|
|||
public class FindCommandParserTest { | |||
|
|||
private static final String EMPTY_STRING = " "; |
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 you can consider renaming this constant to SPACES instead of EMPTY_STRING as an empty string would tend to refer to "" in my opinion.
|
||
private static final String VALID_INDEX_ONE = "1"; | ||
private static final String VALID_INDEX_TWO = "2"; | ||
private static final String EMPTY_STRING = " "; |
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.
Same here.
private String getSerialNumberAsString(Book target) { | ||
return target.getSerialNumber().toString(); | ||
} | ||
|
||
private String getTitleFromBook(Book target) { | ||
return target.getTitle().toString(); | ||
} |
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.
Good job using the toString() method instead of the exact value!
No description provided.