DG update for sort command #148
Conversation
…into permaSort
…into permaSort
| @@ -223,13 +223,26 @@ The following activity diagram summarizes what happens when a user executes a ne | |||
| * **Alternative 2:** Individual command knows how to undo/redo by | |||
| itself. | |||
| * Pros: Will use less memory (e.g. for `delete`, just save the person being deleted). | |||
| * Cons: We must ensure that the implementation of each individual command are correct. | |||
| * Cons: We must ensure that the implementation o=-f each individual command are correct. | |||
There was a problem hiding this comment.
I believe this is a typo
| private static final Comparator<Person> TAG_COMPARATOR = new Comparator<Person>() { | ||
| @Override | ||
| public int compare(Person o1, Person o2) { | ||
| 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); | ||
| } | ||
| }; |
There was a problem hiding this comment.
It seems to me that this comparator has some issues. In particular, the tags are actually stored in Java's Set which has no guarantee of order preservation. So this could result in different comparator results on different calls of the compare method.
There was a problem hiding this comment.
I believe this might be related to the same Comparator in SortContactCommand
There was a problem hiding this comment.
even if it doesn't maintain order preservation, this comparator will always compare the first tag in the set which is also the first tag shown on the contacts display. So it does fulfill what it needs to do?
There was a problem hiding this comment.
Is there a guarantee that the first tag being displayed in the GUI is the same as the first tag found in the set.iterator()?
On a related note, I wonder if this comparator is necessary because it seems quite arbitrary. I was going to suggest perhaps some sort of way to request sorting in the reverse order, so the name comparison would put Z at the front etc. In this case, I think a comparator that allows sorting by number of tags might fit this better. Though I am unsure how this might be useful as well.
There was a problem hiding this comment.
okay noted, will change it to comparing email so users can find all people with similar emails easily. For example, sorting by alphabetical order to find everyone who has the nus prefix email quickly
There was a problem hiding this comment.
Do reflect these changes within the origin SortContactCommand as well. Thanks!
There was a problem hiding this comment.
Yup, please refer to the permaSortCommand PR to see the changes made
| private static final Comparator<Person> NAME_COMPARATOR = new Comparator<Person>() { | ||
| @Override | ||
| public int compare(Person o1, Person o2) { | ||
| return o1.getName().fullName.compareToIgnoreCase(o2.getName().fullName); | ||
| } | ||
| }; | ||
|
|
||
| private static final Comparator<Person> ADDRESS_COMPARATOR = new Comparator<Person>() { | ||
| @Override | ||
| public int compare(Person o1, Person o2) { | ||
| return o1.getAddress().value.compareToIgnoreCase(o2.getAddress().value); | ||
| } | ||
| }; | ||
|
|
||
| private static final Comparator<Person> TAG_COMPARATOR = new Comparator<Person>() { | ||
| @Override | ||
| public int compare(Person o1, Person o2) { | ||
| 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); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Additionally, consider creating a PersonsComparator class which stores all these comparators as public static constants so that both sort commands can refer to the same Comparator object.
| case 2: | ||
| return ADDRESS_COMPARATOR; | ||
| case 3: | ||
| return TAG_COMPARATOR; | ||
| default: | ||
| return NAME_COMPARATOR; |
There was a problem hiding this comment.
consider using case 1: return NAME_COMPATOR;... and throw a CommandException for the default branch. This can potentially safeguard the code from misuse by other developers.
| /** | ||
| * Sorts the address book's internal list according to the given comparator | ||
| */ | ||
| public void sortPerson(Comparator<Person> c) { | ||
| assert persons.asUnmodifiableObservableList().size() > 0; |
There was a problem hiding this comment.
Will this fail if a sort is performed on an empty Athena? Perhaps, it should instead just not do anything if the list size is 0.
| /** | ||
| * Permanently sorts the address book by a specific comparator | ||
| */ | ||
| void sortAddressBook(Comparator<Person> chooseComparator); |
There was a problem hiding this comment.
I believe the name of the method would be more appropriate as something like permSortContacts or permSortPersons as we will eventually need to remove instances of 'addressbook' within the code. Additionally, I believe the argument name would be more appropriate as something like comparator as chooseComparator sounds more like a method name.
| /** | ||
| * Contains unit tests for PermaSortCommand | ||
| */ | ||
| public class PermaSortCommandTest { |
There was a problem hiding this comment.
Would be nice to include test cases that check against the permanently saved file to see if using the command changes it.
There was a problem hiding this comment.
will delete this PR now and push again after permaSort is merged
UML diagram to be added later