soc-pe-bot
commented
6 months ago Team's Response
With reference to the description about the sort persons feature in the UG:
As well as the DG:
The proposed implementation was to make "Sorting articles by date reversible" because "users may want to revert to the previous sorted order to access contacts based on the date they were added into the contact list". I think the latter sentence was a typo and was intended to refer to articles instead.
Firstly, it was mentioned clearly in the UG that sorting articles by date was permanent (UG) and was implemented like so after some considerations on the space overhead it would occur otherwise (DG).
Furthermore, since there is already a proposed implementation of the undo/redo features in the DG:
The reversal of any commands can be made after the proposed features are implemented, which also addresses this suggestion in question.
Lastly, if the user wanted to "access articles based on the date they were added into the article list", I feel that a better way to do so is to simply implement time stamps attached to article entries so that we can sort according to those timestamps to order those entries by recency. Thank you for pointing this out so that we can possibly include such features in our next iteration of the product!
Items for the Tester to Verify
:question: Issue response
Team chose [response.NotInScope]
- [x] I disagree
Reason for disagreement: I appreciate that the dev team has highlighted the "permanent nature" of the sort implementation in the DG and UG, and further explained their considered alternatives regarding this.
Finally, the proposed future undo/redo function is a useful reversion tool for the bug I highlighted to some extent. We should note however, that this solution does not work if the user exits the app and re-enters the app to use it later (unless your undo/redo is implemented as a VCS)
However, the design considerations do not entertain a third possibility that is already implemented in AB3 for the find command: Using the MVC design pattern.
Why MVC
We observe that PressPlanner primarily is used for "storage/retrieval of information, displaying of information to the user (often via multiple UIs having different formats), and changing stored information based on external inputs", which makes the MVC design pattern appropriate to reduce the coupling with uniquePersonList. In the original AB3, modelManager acts as the Model; it stores and maintains data and updates the view when find is used (the filteredPersonList uses the internalUnmodifiableList as a parameter on instantiation). This means that internalList should only be modified through modelManager when articles are added and deleted.
Why having sort modify the internalList directly is not ideal
By having sort directly modify the internalList, PressPlanner couples sort with the classes that directly store the data, instead of modelManager which can make future extensions difficult. In addition, directly manipulating the data stored is not a very safe practice and can incur more performance issues than it avoids. Consider the following:
- User has many articles.
- On sorting articles, all the user's saved data in JSON format is rewritten in sorted order every time a sort command is called.
- Using the proposed redo/undo function will result in the entire JSON file being rewritten over and over again.
Finally, this implementation also means that using the data file to share data between devices running PressPlanner will export this sorted order as well, which may be an unintended side effect.
Conclusion
A review of this bug suggests to me that the feature is poorly implemented due to the significant drawbacks above. Since a more optimal implementation has already been done in AB3 through the find command (sort becomes an extension of the already implemented MVC model), I feel that it does not qualify as "less important (based on the value/effort considerations) than the work that has been done already (because it is fine to delay lower priority work until future iterations)", and therefore should not be classified as response.NotInScope.
Sorting articles by date not Reversible. While it may make sense to sort the dates of articles alphabetically, users may want to revert to the previous sorted order to access contacts based on the date they were added into the contact list.