Add sort command #66

bangyiwu · 2020-10-05T19:27:44Z

First implementation of sorting, sort 1 allows sorting by name, sort 2 allows sorting by address and sort 3 allows sorting by the person's first tag

bangyiwu · 2020-10-05T19:34:24Z

I will fix the cli fails before reopening

LinkedInk · 2020-10-07T04:39:26Z

Model should not handle the sortPerson of the addressbook. Should consider moving the function into the excute part of SortCommand and the class in general. Model class has getAddressBook and setAddressBook to help with that.

chan-j-d · 2020-10-07T09:54:02Z

Is the sort method supposed to sort the current shown list? Or is it supposed to sort the full list of contacts before showing it? Because the former would be much easier to implement and I believe is what we intended to do originally? There is the model.getFilteredPersonList() method which gives the last shown list.

Also, I agree with the Hendey's comment about the sortPerson method. I do not think that all the model classes need to have such a method.

codecov-io · 2020-10-08T07:15:02Z

Codecov Report

Merging #66 into master will decrease coverage by 1.75%.
The diff coverage is 10.90%.

@@             Coverage Diff              @@
##             master      #66      +/-   ##
============================================
- Coverage     61.00%   59.25%   -1.76%     
  Complexity      457      457              
============================================
  Files            89       91       +2     
  Lines          1631     1681      +50     
  Branches        187      193       +6     
============================================
+ Hits            995      996       +1     
- Misses          586      635      +49     
  Partials         50       50

Impacted Files	Coverage Δ	Complexity Δ
...java/seedu/address/logic/commands/SortCommand.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (?)`
.../seedu/address/logic/parser/AddressBookParser.java	`69.56% <0.00%> (-3.17%)`	`12.00 <0.00> (ø)`
.../seedu/address/logic/parser/SortCommandParser.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (?)`
src/main/java/seedu/address/model/AddressBook.java	`84.61% <0.00%> (-7.06%)`	`12.00 <0.00> (ø)`
src/main/java/seedu/address/model/Model.java	`100.00% <ø> (ø)`	`1.00 <0.00> (ø)`
...a/seedu/address/model/person/UniquePersonList.java	`88.37% <0.00%> (-4.32%)`	`20.00 <0.00> (ø)`
...rc/main/java/seedu/address/model/ModelManager.java	`73.52% <50.00%> (-1.86%)`	`23.00 <1.00> (ø)`
...rc/main/java/seedu/address/logic/LogicManager.java	`65.21% <100.00%> (ø)`	`3.00 <1.00> (ø)`
...va/seedu/address/logic/commands/DeleteCommand.java	`100.00% <100.00%> (ø)`	`7.00 <0.00> (ø)`
...java/seedu/address/logic/commands/EditCommand.java	`91.66% <100.00%> (ø)`	`16.00 <0.00> (ø)`
... and 3 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 52edc97...6de30ea. Read the comment docs.

This is to preserve the original tag options. The edit command will then only specify what tags to add and remove. This is also for future-proofing as we intend to add support for searching by tags.

…tCommand

bangyiwu · 2020-10-12T16:24:37Z

Challenges faced: so far only able to sort the address book permanently, if anyone has an idea on how we can sort the address book temporarily, let me know

SortedList is used to wrap around the FilteredList as the FilteredList itself is immutable. The getFilteredPersonList() has been changed to return the sortedList instead. This makes it such that both filters and comparators are reflected in the list being shown.

…tCommand

chan-j-d

Please take a look at the suggestions

chan-j-d · 2020-10-13T17:22:05Z

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

@@ -11,6 +11,7 @@
    public static final Prefix PREFIX_EMAIL = new Prefix("e/");
    public static final Prefix PREFIX_ADDRESS = new Prefix("a/");
    public static final Prefix PREFIX_TAG = new Prefix("t/");
+    public static final Prefix PREFIX_SORT = new Prefix("s/");


Is the prefix necessary at all?

chan-j-d · 2020-10-13T17:22:44Z

src/main/java/seedu/address/logic/parser/SortCommandParser.java

+        requireNonNull(args);
+        ArgumentMultimap argMultimap = ArgumentTokenizer.tokenize(args, PREFIX_SORT);
+
+        Index index;
+        try {
+            index = ParserUtil.parseIndex(argMultimap.getPreamble());


Should be able to obtain the index directly similar to the Delete command for example

chan-j-d · 2020-10-13T17:23:10Z

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

+            + "by the index command entered "
+            + "1 will be sort by date added\n"
+            + "2 will be sort by alphabetical order of their names\n"
+            + "3 will be sort by alphabetical order of their address\n"
+            + "4 will be sort by alphabetical order of their first tag\n"


This seems to be outdated compared to what is actually implemented

chan-j-d · 2020-10-13T17:23:25Z

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

+import seedu.address.commons.core.index.Index;
+import seedu.address.logic.commands.exceptions.CommandException;
+import seedu.address.model.Model;
+


Would be nice to have a brief Javadocs description of the class

chan-j-d · 2020-10-13T17:24:52Z

src/main/java/seedu/address/logic/parser/SortCommandParser.java

+import seedu.address.logic.parser.exceptions.ParseException;
+
+/**
+ * Parses input arguments and creates a new {@code RemarkCommand} object


This Javadocs is outdated

chan-j-d · 2020-10-13T17:28:09Z

src/main/java/seedu/address/model/person/UniquePersonList.java

+    public void sortPersons(Comparator<Person> c) {
+        internalList.sort(c);
+    }


Should probably remove this as it is only used by the unused sortPersons method in AddressBook

chan-j-d · 2020-10-13T17:29:11Z

src/test/java/seedu/address/logic/commands/AddCommandTest.java

+        public void sortPerson(Index index) {
+            throw new AssertionError("This method should not be called.");
+        }


Should probably remove this as it is not used at all

chan-j-d · 2020-10-13T17:30:58Z

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

@@ -156,15 +163,58 @@ public void setPerson(Person target, Person editedPerson) {
        addressBook.setPerson(target, editedPerson);
    }

+    @Override
+    public void sortPerson(Index index) {


This method should probably take in a Comparator instead in line with how the updateFilteredPersonList method works. Model Manager shouldn't need to know exactly which Comparators it needs

chan-j-d · 2020-10-13T17:33:55Z

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

+        Comparator<Person> nameComparator = new Comparator<Person>() {
+            @Override
+            public int compare(Person o1, Person o2) {
+                return o1.getName().fullName.compareToIgnoreCase(o2.getName().fullName);
+            }
+        };
+
+        Comparator<Person> addressComparator = new Comparator<Person>() {
+            @Override
+            public int compare(Person o1, Person o2) {
+                return o1.getAddress().value.compareToIgnoreCase(o2.getAddress().value);
+            }
+        };
+
+        Comparator<Person> tagComparator = new Comparator<Person>() {
+            @Override
+            public int compare(Person o1, Person o2) {
+                System.out.println(o2.getTags().size());
+                if (o1.getTags().size() == 0 && o2.getTags().size() != 0) {
+                    return 1;
+                }
+                if (o1.getTags().size() != 0 && o2.getTags().size() == 0) {
+                    return -1;
+                }
+                if (o1.getTags().size() == 0 && o2.getTags().size() == 0) {
+                    return o1.getName().fullName.compareToIgnoreCase(o2.getName().fullName);
+                }
+                return o1.getTags().iterator().next().tagName
+                        .compareToIgnoreCase(o2.getTags().iterator().next().tagName);
+            }
+        };


The three comparators should probably declared as static constants within the SortCommand class itself?

chan-j-d · 2020-10-13T17:34:17Z

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

+        if (indexNumber == 1) {
+            sortedPersons.comparatorProperty().setValue(nameComparator);
+        } else if (indexNumber == 2) {
+            sortedPersons.comparatorProperty().setValue(addressComparator);
+        } else if (indexNumber == 3) {
+            sortedPersons.comparatorProperty().setValue(tagComparator);
+        }


This should probably be done in the SortCommand class itself. execute chooses the appropriate Comparator in the execute command to pass to ModelManager.

chan-j-d

LGTM!

bangyiwu added 2 commits October 5, 2020 16:32

Add Sort Command

cc2ac8d

Add Sort Command 2nd Implementation

ef9c3da

bangyiwu closed this Oct 5, 2020

Fix CLI fail

5dc570c

bangyiwu reopened this Oct 5, 2020

bangyiwu closed this Oct 5, 2020

bangyiwu reopened this Oct 5, 2020

bangyiwu added 5 commits October 6, 2020 03:50

Fix CLI fails v2

aa53709

Fix checkstyle

dabe993

Fix model stub

be9920b

Fix model stub v2

83be58d

Fix model stub v3

c16d8a2

bangyiwu added this to the v1.2 milestone Oct 6, 2020

bangyiwu linked an issue Oct 6, 2020 that may be closed by this pull request

As a user, I can sort my contacts #11

Closed

Merge branch 'master' into sortCommand

8d6bdf3

bangyiwu linked an issue Oct 8, 2020 that may be closed by this pull request

Implement sorting function #72

Closed

bangyiwu removed a link to an issue Oct 8, 2020

As a user, I can sort my contacts #11

Closed

bangyiwu and others added 9 commits October 12, 2020 23:54

Update DG

3047e76

Update DeveloperGuide.md

afc1f6f

Change EditPersonDescription to contain add and remove tag options

db83669

This is to preserve the original tag options. The edit command will then only specify what tags to add and remove. This is also for future-proofing as we intend to add support for searching by tags.

Implement rt/* to remove all tags

5f68c4d

Update Javadocs for tag methods that have changed in logic

f37d592

Update EditPersonDescriptionBuilder to use tagsToAdd and tagsToRemove

8574da1

Update test cases to test new add and remove implementation

ff987b1

Fix checkstyle issues

5a6dcb7

Fix additional tests broken by changes in tag logic

66b720b

bangyiwu added 3 commits October 12, 2020 23:57

Merge branch 'sortCommand' of https://github.com/bangyiwu/tp into sor…

05d5129

…tCommand

Merge branch 'master' into sortCommand

65d3fce

Fix letter case error

a76a27d

bangyiwu and others added 13 commits October 13, 2020 21:55

update from master

8e852d5

Edit import statements

a97a5dd

Fix Checkstyle errors

d85f26b

Merge branch 'master' into sortCommand

981e245

Fix Checkstyle v2

2802080

Merge branch 'sortCommand' of https://github.com/bangyiwu/tp into sor…

8adf63a

…tCommand

getSortedFilteredPersonList

8ed6b99

Fix logic manager getSortedFilteredPerson

178dba6

Update main window

81f7296

Fix references to filterlist

6068586

Fix references to filterlist v2

dcb8264

Fix messages and tagComparator bug

7bf9686

bangyiwu requested a review from chan-j-d October 13, 2020 17:15

chan-j-d requested changes Oct 13, 2020

View reviewed changes

bangyiwu added 5 commits October 14, 2020 02:13

Improve javadocs and OOP

1ae7b91

Fix checkstyle

aaa53c5

Fix checkstyle 2

b4db616

Fix model stub for addCommandTest

fc099c5

Fix model stub for addCommandTest 2

6de30ea

chan-j-d linked an issue Oct 14, 2020 that may be closed by this pull request

As a user, I can sort my contacts #11

Closed

chan-j-d approved these changes Oct 14, 2020

View reviewed changes

chan-j-d merged commit 2d273f7 into AY2021S1-CS2103T-W10-4:master Oct 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add sort command #66

Add sort command #66

bangyiwu commented Oct 5, 2020

bangyiwu commented Oct 5, 2020

LinkedInk commented Oct 7, 2020

chan-j-d commented Oct 7, 2020

codecov-io commented Oct 8, 2020 •

edited

bangyiwu commented Oct 12, 2020

chan-j-d left a comment

chan-j-d Oct 13, 2020

chan-j-d Oct 13, 2020

chan-j-d Oct 13, 2020

chan-j-d Oct 13, 2020

chan-j-d Oct 13, 2020

chan-j-d Oct 13, 2020

chan-j-d Oct 13, 2020

chan-j-d Oct 13, 2020

chan-j-d Oct 13, 2020

chan-j-d Oct 13, 2020

chan-j-d left a comment

Add sort command #66

Add sort command #66

Conversation

bangyiwu commented Oct 5, 2020

bangyiwu commented Oct 5, 2020

LinkedInk commented Oct 7, 2020

chan-j-d commented Oct 7, 2020

codecov-io commented Oct 8, 2020 • edited

Codecov Report

bangyiwu commented Oct 12, 2020

chan-j-d 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

Choose a reason for hiding this comment

chan-j-d left a comment

Choose a reason for hiding this comment

codecov-io commented Oct 8, 2020 •

edited