Branch suggestion #182
Branch suggestion #182
Conversation
tankangliang
requested review from
qwoprocks and
raysonkoh
and removed request for
qwoprocks
Codecov Report
@@ Coverage Diff @@
## master #182 +/- ##
============================================
- Coverage 72.43% 71.96% -0.47%
- Complexity 533 545 +12
============================================
Files 90 93 +3
Lines 1665 1705 +40
Branches 171 176 +5
============================================
+ Hits 1206 1227 +21
- Misses 401 419 +18
- Partials 58 59 +1
Continue to review full report at Codecov.
|
docs/UserGuide.md
Outdated
|
|
||
| Format: `suggestion by/SUGGESTION_TYPE [by/SUGGESTION_TYPE]` | ||
|
|
||
| * SUGGESTION_TYPE must be one of the following: `contact`, `available` or `priority` |
There was a problem hiding this comment.
| * SUGGESTION_TYPE must be one of the following: `contact`, `available` or `priority` | |
| * SUGGESTION_TYPE must be one of the following: `frequency`, `available` or `contract` |
docs/UserGuide.md
Outdated
|
|
||
| Obtains a list of clients based on the suggestion type(s) passed in. | ||
|
|
||
| Format: `suggestion by/SUGGESTION_TYPE [by/SUGGESTION_TYPE]` |
There was a problem hiding this comment.
maybe change to command word suggest instead? Make it seem more like normal english
| * Returns true if a given string is a valid tag name. | ||
| */ | ||
| public static boolean isValidSuggestionType(String test) { | ||
| return test.equals("contact") || test.equals("available") || test.equals("priority"); |
There was a problem hiding this comment.
should extract these strings as static final vars
| public CommandResult execute(Model model) { | ||
| requireNonNull(model); | ||
| return new CommandResult(MESSAGE_SUGGESTION_SUCCESS); | ||
| } |
There was a problem hiding this comment.
Should add a TODO here to do something in the execution
docs/UserGuide.md
Outdated
| * `suggest by/available` Obtains a list of clients where the time is 1800-2200 in the client's timezone (off work hours). | ||
| * `suggest by/frequency` Obtains a list of clients based on the last time their details were edited in TBM. Clients who have not been contacted for a longer period will be the first in the list. | ||
| * `suggest by/contract` Obtains a list of clients based on their current contract details. Clients whose contracts are expiring will be shown first. | ||
| * `suggest by/contract by/available` Similar to `suggestion by/priority` but only available clients will be shown. |
There was a problem hiding this comment.
Do you mean "Similar to suggestion by/contract" ?
There was a problem hiding this comment.
yea forgot to change it when i updated to mingchong's changes :(
|
|
||
|
|
||
| /** | ||
| * Parses {@code Collection<String> tags} into a {@code Set<Tag>}. |
There was a problem hiding this comment.
Possible typo. Should Collection<String> tags be Collection<String> suggestionTypes instead? Similar for the Set<Tag>.
| * Represents a SuggestionType in the address book. | ||
| * Guarantees: immutable; suggestion type is valid as declared in {@link #isValidSuggestionType(String)} | ||
| */ | ||
| public class SuggestionType { |
There was a problem hiding this comment.
Just a suggestion, should SuggestionType be an Enum instead? Since it is only taking frequency, available and contract as valid inputs.
There was a problem hiding this comment.
possible, I initially put it as an enum but it seemed too different from the rest of the code base. What do you think @qwoprocks
There was a problem hiding this comment.
I think it's fine to leave it as a class for now, since if we change it to an enum it might be hard to integrate. Maybe can just KIV as a TODO.
Description
Draft of the suggestion command that will be implemented in v1.3
Testing
NIL
Remarks
This current implementation obtains a set of
SuggestionTypewhich has to be manually checked for insideSuggestionCommandbefore applying the necessary modifications to the client list.I was thinking we could also have
SuggestionTypeas an interface similar toCommandand having the 3 types of suggestions as classes that implement the interface with a method that applies a modification to the client list.