List and Find Command #198
Conversation
…into list-command
…into list-command
seanjyjy
requested review from
ZoroarkDarkrai,
orzymandias,
keanecjy and
shawn-nyk
Codecov Report
@@ Coverage Diff @@
## master #198 +/- ##
============================================
+ Coverage 49.35% 49.81% +0.45%
- Complexity 772 794 +22
============================================
Files 185 187 +2
Lines 3432 3485 +53
Branches 394 394
============================================
+ Hits 1694 1736 +42
- Misses 1636 1644 +8
- Partials 102 105 +3
Continue to review full report at Codecov.
|
seanjyjy
requested review from
a team
and removed request for
ZoroarkDarkrai,
orzymandias and
keanecjy
There was a problem hiding this comment.
Great work! Apart from the minor comments made, will wait to hear the reply on how we should be using String constants from the GeneralStringUtil class. [My take as discussed is that perhaps we should use it only when they appear in code that is not part of defining yet another String constant (e.g. we should use it in line 106 of Card.java, as opposed to the MESSAGE_USAGE definitions of the various commands, since they are themselves String constants, in which using other String constants should be used only for consistency purposes).]
|
|
||
| public static final String COMMAND_WORD = "list"; | ||
|
|
||
| public static final String MESSAGE_SUCCESS = "Listed all applications"; |
There was a problem hiding this comment.
As all our list commands are going to have similar success messages, perhaps we could do what we did with the rest of the commands which would be to add something like this after line 25: String listSuccessMessage = String.format(MESSAGE_LISTED_ITEMS, APPLICATION_NAME); where MESSAGE_LISTED_ITEMS resides in Messages.java and goes something like: "Listed all %1$s items";? Just a suggestion!
There was a problem hiding this comment.
oh this sounds great, will abstract it into messages.java then
| */ | ||
| public class ListApplicationCommand extends ListCommand { | ||
|
|
||
| public static final String COMMAND_WORD = "list"; |
There was a problem hiding this comment.
Is this redundant given that ListApplicationCommand can use ListCommand's COMMAND_WORD? This is with regards to how all the other commands exhibit the same pattern in that they use the parent's COMMAND_WORD.
There was a problem hiding this comment.
Ah, yes that is true, will make the relevant changes once we get our affirmation regarding the above dicussions
…into list-command � Conflicts: � src/main/java/seedu/address/logic/parser/util/GeneralParserUtil.java
…into list-command � Conflicts: � src/main/java/seedu/address/logic/commands/find/FindApplicationCommand.java � src/test/java/seedu/address/testutil/application/SampleApplicationItems.java
| /** Find message: First string refers to the number if item found */ | ||
| public static final String MESSAGE_FIND_APPLICATION_SUCCESS = "%1$d applications listed!"; | ||
| /** Find message: First string refers to the number if item found. second string refers to the item type. */ | ||
| public static final String MESSAGE_FIND_SUCCESS = "%1$d %2$s listed!"; |
There was a problem hiding this comment.
Super minor point, but with regards to the phrasing, I suppose this might read a little awkwardly for more than 1 item found, such as "3 company listed!"? But it's just the phrasing so it's not a big deal and we can probably handle this in the future if we want!
There was a problem hiding this comment.
yeah i did thought about it, but it feels weird if we have to add in a condition for it LOL for whether to display the extra s. Hmm i think since we are dealing with a 1 vs Infinite amount, it feels like having the s should make sense right O:
There was a problem hiding this comment.
I think I will temporarily put this as %1$d %2$s items found!
| @@ -30,15 +29,9 @@ public static TabName getSwitchTabName(String tab) throws ParseException { | |||
| tabName = TabName.PROFILE; | |||
| break; | |||
| default: | |||
| throw new ParseException(MESSAGE_INVALID_TAB); | |||
| throw new ParseException(MESSAGE_INVALID_ITEM_TYPE); | |||
There was a problem hiding this comment.
Will this throw an inappropriate error message? Correct me if I'm mistaken, but does this throw the error message "Item type has to be either 'com', 'int', 'app' or 'me'"? If so, I believe that since we can't run switch int, should we revert back to the old error message / something along the lines of "Item type has to be either 'com', 'app' or 'me'"?
There was a problem hiding this comment.
hmm, I think since this is a more towards only switch kind of command, I believe I should make a separate message then. Are there any other commands that have the same issue? I believe list and find as well right?
src/main/java/seedu/address/logic/commands/list/ListApplicationCommand.java
Outdated
Show resolved
Hide resolved
Pr Overview
ListApplicationCommandandListApplicationCommandTestFindApplicationCommandandListApplicationCommandTestGeneralStringUtilMainParserTestso that it looks neater.