Refactor classes #165
Refactor classes #165
Conversation
chunweii
added
priority.High
Codecov Report
@@ Coverage Diff @@
## master #165 +/- ##
============================================
- Coverage 65.59% 64.90% -0.70%
+ Complexity 942 927 -15
============================================
Files 124 126 +2
Lines 3305 3294 -11
Branches 422 428 +6
============================================
- Hits 2168 2138 -30
- Misses 964 965 +1
- Partials 173 191 +18
Continue to review full report at Codecov.
|
| this.predicate = predicate; | ||
| } | ||
|
|
||
| @Override | ||
| public CommandResult execute(Model model) { | ||
| requireNonNull(model); | ||
| model.setContactDisplaySetting(ContactDisplaySetting.DEFAULT_SETTING); |
There was a problem hiding this comment.
is this line necessary? if we call clist a/ then cfind, this would change the display to the default one? our current implementation is to keep the same display of clist i think
There was a problem hiding this comment.
The problem with such an implementation is that if the user calls the view commands earlier, then if they call the find command, the full details would be shown.
| this.willDisplayAddress = willDisplayAddress; | ||
| this.willDisplayZoomLink = willDisplayZoomLink; | ||
| this.willDisplayTags = willDisplayTags; | ||
| this.isViewingFull = false; |
There was a problem hiding this comment.
would it be more logical to move this setting to be inside the individual contact instead of the whole contact list? but if this works for cview and eview, i guess its okay
There was a problem hiding this comment.
This is very similar to the old implementation, whereby the settings are stored as static variables. It is possible to have the individual contacts and events store these settings. But since we are trying to make contacts and events immutable, I think there will be a major performance cost involved, as each time we call those commands which will change the display settings, all events and contacts will need to be replaced.
There was a problem hiding this comment.
hmm if we only move the isViewing setting to be managed by individual contact/event, we would only need to re-create the specific contact/event for cview/eview right? but i also feel its a bit weird if only isViewing setting is separated from the rest of the setting. haha but yeah i think ur current implementation is fine too 👍
Closes #163