Branch improve sort command #119

angrybunny123 · 2020-10-23T17:02:02Z

Closes #120

Added Sorting Preference field to user preferences, and changed occurrences of addressbook.json to library.json.

Can sort by reading progress using prefix "rp/"

Using the sort command now updates user preference as well, and when the user exits and reopens bookmark, books will still be sorted in the specified order.

Edit and Add book commands will be based on sorting preference of the user.

…o branch-improve-sort-command

TanLeYang · 2020-10-24T14:03:48Z

src/main/java/seedu/bookmark/model/ModelManager.java

@@ -142,9 +173,62 @@ public void updateFilteredBookList(Predicate<Book> predicate) {

    @Override
    public void sortFilteredBookList(Comparator<Book> comparator) {
+        if (comparator instanceof BookNameComparator) {
+            setSortingPreference(PREFIX_NAME.toString());


Should we be doing these checks here? An approach worth thinking about could be to get SortCommandParser to create each SortCommands that know what prefix it's sorting by. Then in SortCommand#execute() we can just call setSortingPreference(PREFIX_XXX)

Thanks for the suggestions Le Yang, I agree with you and I have made the necessary changes 😄

TanLeYang · 2020-10-24T14:07:25Z

src/main/java/seedu/bookmark/model/ModelManager.java

+     * Returns a comparator based on the input prefix
+     * @return Comparator based on input prefix
+     */
+    public Comparator<Book> comparatorGenerator(Prefix inputPrefix) {


Noticed that this exact method is repeated inside SortCommandParser also. Can think about abstracting it out as a static utility method, maybe smth like BookComparatorGenerator#comparatorGenerator(Prefix inputPrefix)

TanLeYang · 2020-10-24T14:16:57Z

src/main/java/seedu/bookmark/model/ModelManager.java

+     * Returns a prefix based on the input string.
+     * @return Prefix based on the input string.
+     */
+    public Prefix prefixGenerator(String prefix) {


I don't think this method belongs here, maybe define it as a utility method in CliSyntax ?

codecov-io · 2020-10-25T03:57:26Z

Codecov Report

Merging #119 into master will decrease coverage by 0.03%.
The diff coverage is 76.05%.

@@             Coverage Diff              @@
##             master     #119      +/-   ##
============================================
- Coverage     74.10%   74.07%   -0.04%     
- Complexity      669      685      +16     
============================================
  Files           105      107       +2     
  Lines          2078     2133      +55     
  Branches        238      242       +4     
============================================
+ Hits           1540     1580      +40     
- Misses          455      465      +10     
- Partials         83       88       +5

Impacted Files	Coverage Δ	Complexity Δ
src/main/java/seedu/bookmark/model/Model.java	`100.00% <ø> (ø)`	`1.00 <0.00> (ø)`
...in/java/seedu/bookmark/logic/parser/CliSyntax.java	`58.33% <25.00%> (-33.34%)`	`2.00 <1.00> (+1.00)`	⬇️
src/main/java/seedu/bookmark/model/UserPrefs.java	`76.92% <70.00%> (-3.08%)`	`14.00 <3.00> (+4.00)`	⬇️
...ava/seedu/bookmark/logic/commands/SortCommand.java	`80.00% <71.42%> (ø)`	`4.00 <1.00> (ø)`
...c/main/java/seedu/bookmark/model/ModelManager.java	`97.89% <90.00%> (-0.93%)`	`35.00 <4.00> (+3.00)`	⬇️
...ook/comparators/BookReadingProgressComparator.java	`90.00% <90.00%> (ø)`	`4.00 <4.00> (?)`
...rk/model/book/comparators/ComparatorGenerator.java	`90.90% <90.90%> (ø)`	`5.00 <5.00> (?)`
...ava/seedu/bookmark/logic/commands/EditCommand.java	`95.77% <100.00%> (+0.18%)`	`14.00 <1.00> (+1.00)`
...seedu/bookmark/logic/parser/SortCommandParser.java	`88.00% <100.00%> (+1.33%)`	`7.00 <0.00> (-2.00)`	⬆️
...rk/model/book/comparators/BookGenreComparator.java	`83.33% <100.00%> (ø)`	`4.00 <1.00> (ø)`
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 92d727b...5756611. Read the comment docs.

TanLeYang

LGTM!

pennhanlee · 2020-10-25T04:57:57Z

docs/tutorials/AddRemark.md

@@ -292,7 +292,7 @@ While the changes to code may be minimal, the test data will have to be updated

 <div markdown="span" class="alert alert-warning">

-:exclamation: You must delete AddressBook’s storage file located at `/data/addressbook.json` before running it! Not doing so will cause AddressBook to default to an empty address book!
+:exclamation: You must delete AddressBook’s storage file located at `/data/library.json` before running it! Not doing so will cause AddressBook to default to an empty address book!


Perhaps you could change AddressBook to Library for this part.

pennhanlee · 2020-10-25T04:58:54Z

docs/tutorials/RemovingFields.md

@@ -87,7 +87,7 @@ At this point, your application is working as intended and all your tests are pa

 In `src/test/data/`, data meant for testing purposes are stored. While keeping the `address` field in the json files does not cause the tests to fail, it is not good practice to let cruft from old features accumulate.


Perhaps you can change Address so Genre for this part!

pennhanlee · 2020-10-25T05:04:50Z

src/main/java/seedu/bookmark/model/book/comparators/BookReadingProgressComparator.java

+    @Override
+    public String toString() {
+        return "Reading Progress";
+    }


Maybe the toString() return value can be more meaningful?

I use this toString() value to return "Sorted by: xxxx.toString()", so I think leaving it like this is fine

pennhanlee · 2020-10-25T05:06:29Z

src/test/java/seedu/bookmark/logic/LogicManagerTest.java

@@ -71,7 +71,7 @@ public void execute_validCommand_success() throws Exception {
    public void execute_storageThrowsIoException_throwsCommandException() {
        // Setup LogicManager with JsonAddressBookIoExceptionThrowingStub


Perhap you could change this to JsonLibraryBook... ?

pennhanlee · 2020-10-25T05:06:48Z

src/test/java/seedu/bookmark/logic/LogicManagerTest.java

@@ -71,7 +71,7 @@ public void execute_validCommand_success() throws Exception {
    public void execute_storageThrowsIoException_throwsCommandException() {
        // Setup LogicManager with JsonAddressBookIoExceptionThrowingStub
        JsonLibraryStorage addressBookStorage =


Perhaps you can change this to libraryStorage?

pennhanlee · 2020-10-25T05:08:09Z

src/main/java/seedu/bookmark/logic/parser/CliSyntax.java

+        default:
+            return PREFIX_DUMMY;
+        }


Maybe it will be better if we throw an exception instead of defaulting to a dummy sorting choice?

Exception would be thrown in SortCommandParser if prefix was invalid. But I'll change this to return null in the default case to make it less confusing.

pennhanlee · 2020-10-25T13:03:57Z

docs/tutorials/AddRemark.md

@@ -292,7 +292,7 @@ While the changes to code may be minimal, the test data will have to be updated

 <div markdown="span" class="alert alert-warning">

-:exclamation: You must delete AddressBook’s storage file located at `/data/addressbook.json` before running it! Not doing so will cause AddressBook to default to an empty address book!
+:exclamation: You must delete bookmark’s storage file located at `/data/library.json` before running it! Not doing so will cause AddressBook to default to an empty address book!


Did you miss out 2 AddressBook at the end of the sentence?

pangpuncake

Overall LGTM! Just have a question on the use of String instead of Prefix in UserPrefs.

pangpuncake · 2020-10-25T14:40:52Z

src/main/java/seedu/bookmark/logic/commands/SortCommand.java

+    public SortCommand(Comparator<Book> comparator, Prefix inputPrefix) {
        this.comparator = comparator;
+        this.inputPrefix = inputPrefix;


Perhaps usage of requireNonNull for more defensive code.

pangpuncake · 2020-10-25T14:49:26Z

docs/tutorials/RemovingFields.md

@@ -87,7 +87,7 @@ At this point, your application is working as intended and all your tests are pa

 In `src/test/data/`, data meant for testing purposes are stored. While keeping the `address` field in the json files does not cause the tests to fail, it is not good practice to let cruft from old features accumulate.

-**`invalidPersonAddressBook.json`:**
+**`invalidPersonLibrary.json`:**


This should be

Suggested change

**`invalidPersonLibrary.json`:**

**`invalidBookLibrary.json`:**

pangpuncake · 2020-10-25T15:03:38Z

src/main/java/seedu/bookmark/model/UserPrefs.java

@@ -15,6 +15,7 @@

    private GuiSettings guiSettings = new GuiSettings();
    private Path bookmarkFilePath = Paths.get("data" , "library.json");
+    private String sortingPreference = "";


You might want to consider using Prefix instead of String. From what I see, you are converting from Prefix to String to set in UserPrefs. In order to get back the Prefix to create a Comparator, you have to convert the String back to a Prefix which seems unnecessary if Prefix was used instead. By using Prefix, you can remove the need for sortingPrefixGenerator and reduce the possibilities of bugs by the back and forth conversions.
Is there any additional benefit to using a String that I have missed out?

pangpuncake · 2020-10-25T15:07:34Z

src/main/java/seedu/bookmark/model/ModelManager.java

@@ -36,6 +39,7 @@ public ModelManager(ReadOnlyLibrary library, ReadOnlyUserPrefs userPrefs) {

        this.library = new Library(library);
        this.userPrefs = new UserPrefs(userPrefs);
+        this.comparator = comparatorGenerator(sortingPrefixGenerator(userPrefs.getSortingPreference()));


Relating to my comment on using a Prefix instead of String in UserPrefs below, a Prefix would remove the need for sortingPrefixGenerator(). This line seems to have too much chaining.

…ybunny123/tp into branch-improve-sort-command

pennhanlee

LGTM!

TanLeYang · 2020-10-26T09:42:49Z

src/main/java/seedu/bookmark/model/ModelManager.java

@@ -180,6 +202,7 @@ public void updateFilteredBookList(Predicate<Book> predicate) {

    @Override
    public void sortFilteredBookList(Comparator<Book> comparator) {
+        this.comparator = comparator;
        this.library.sortBooks(comparator);
    }


Could you add this code to the end of the method
historyManager = historyManager.addNewState(State.createState(library, userPrefs, filteredBooks.getPredicate()));
to make the sort command undoable?

angrybunny123 added 10 commits October 23, 2020 16:57

Fix Book Genre & Name comparator bug

3bfe6cd

Add BookReadingProgressComparator and prefix syntax

de60b93

Change addressbook.json to library.json

96cdb7d

Add sorting preference field to userPrefs

674fb12

Modify edit, add commands to adhere to sorting rule

63e4c2b

Finish improved sort implementation for src folder

0b9e56f

Edit comparator to be null if no sort preference

a89b997

Edit SortCommandTest to test for sort then add book

741d012

Edit UserPrefsTest

e869479

Update User Guide for Sort Command

5cacb81

angrybunny123 added this to the v1.3 milestone Oct 23, 2020

angrybunny123 requested review from mgiang2015, TanLeYang, pangpuncake and pennhanlee October 23, 2020 17:02

angrybunny123 and others added 5 commits October 24, 2020 01:09

Merge branch 'master' into branch-improve-sort-command

323a58f

Update User Guide for Sort Command

98dded4

Merge branch 'master' of https://github.com/AY2021S1-CS2103T-F13-2/tp

af566da

Add Assertions in UserPrefs

1f5953f

Merge remote-tracking branch 'origin/branch-improve-sort-command' int…

ec65f5a

…o branch-improve-sort-command

TanLeYang reviewed Oct 24, 2020

View reviewed changes

angrybunny123 added 3 commits October 25, 2020 11:38

Add input prefix to sort command constructor

966b822

Abstracted prefixGenerator and ComparatorGenerator

b87d2c7

Adhere to checkstyle

0306efc

TanLeYang approved these changes Oct 25, 2020

View reviewed changes

pennhanlee requested changes Oct 25, 2020

View reviewed changes

angrybunny123 added 2 commits October 25, 2020 14:39

Minor changes

c42b9b5

Minor changes

a4be17b

pennhanlee reviewed Oct 25, 2020

View reviewed changes

pangpuncake requested changes Oct 25, 2020

View reviewed changes

angrybunny123 and others added 9 commits October 25, 2020 23:34

Minor changes

bdd1e98

Minor changes

f9aff7c

Merge branch 'master' into branch-improve-sort-command

44fb9f8

Merge branch 'master' of https://github.com/AY2021S1-CS2103T-F13-2/tp

325017d

Update EditCommand.java

64ec9bf

Update EditCommand.java

c44d1df

Update EditCommand.java

5756611

Merge conflict settled

1754190

Merge branch 'branch-improve-sort-command' of https://github.com/angr…

f03a145

…ybunny123/tp into branch-improve-sort-command

pennhanlee reviewed Oct 26, 2020

View reviewed changes

pennhanlee approved these changes Oct 26, 2020

View reviewed changes

TanLeYang reviewed Oct 26, 2020

View reviewed changes

angrybunny123 and others added 2 commits October 26, 2020 18:45

Make command undoable

7f64f6e

Merge branch 'master' into branch-improve-sort-command

2d17bc3

TanLeYang merged commit 811f25a into AY2021S1-CS2103T-F13-2:master Oct 26, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Branch improve sort command #119

Branch improve sort command #119

angrybunny123 commented Oct 23, 2020 •

edited

TanLeYang Oct 24, 2020

angrybunny123 Oct 25, 2020

TanLeYang Oct 24, 2020

TanLeYang Oct 24, 2020

codecov-io commented Oct 25, 2020 •

edited

TanLeYang left a comment

pennhanlee Oct 25, 2020

pennhanlee Oct 25, 2020

pennhanlee Oct 25, 2020

angrybunny123 Oct 25, 2020

pennhanlee Oct 25, 2020

pennhanlee Oct 25, 2020

pennhanlee Oct 25, 2020

angrybunny123 Oct 25, 2020

pennhanlee Oct 25, 2020

pangpuncake left a comment

pangpuncake Oct 25, 2020

pangpuncake Oct 25, 2020

pangpuncake Oct 25, 2020

pangpuncake Oct 25, 2020

pennhanlee left a comment

TanLeYang Oct 26, 2020

		@@ -87,7 +87,7 @@ At this point, your application is working as intended and all your tests are pa

		In `src/test/data/`, data meant for testing purposes are stored. While keeping the `address` field in the json files does not cause the tests to fail, it is not good practice to let cruft from old features accumulate.

		@@ -71,7 +71,7 @@ public void execute_validCommand_success() throws Exception {
		public void execute_storageThrowsIoException_throwsCommandException() {
		// Setup LogicManager with JsonAddressBookIoExceptionThrowingStub

	`invalidPersonLibrary.json`:
	`invalidBookLibrary.json`:

Branch improve sort command #119

Branch improve sort command #119

Conversation

angrybunny123 commented Oct 23, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Oct 25, 2020 • edited

Codecov Report

TanLeYang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pangpuncake left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pennhanlee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

angrybunny123 commented Oct 23, 2020 •

edited

codecov-io commented Oct 25, 2020 •

edited