Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Sort feature #53

Merged
merged 12 commits into from
Oct 14, 2020

Conversation

kkangs0226
Copy link
Collaborator

List of projects displayed cannot be sorted in a specified order.

Adding sort feature will make application more user friendly as it
can display projects in orders required by users.

Let's

  • add SortCommand and SortCommandParser
  • add other relevant Comparators for sorting
  • make necessary changes to existing files
  • add tests for SortCommand, SortCommandParser and Comparators

Closes #47, closes #48

List of projects displayed cannot be sorted in a specified order.

Adding sort feature will make application more user friendly as it
can display projects in orders required by users.

Let's
* add SortCommand and SortCommandParser
* add other relevant Comparators for sorting
* make necessary changes to existing files
* add tests for SortCommand, SortCommandParser and Comparators
@kkangs0226 kkangs0226 added the priority.Medium Nice to have label Oct 14, 2020
@kkangs0226 kkangs0226 added this to the v1.2 milestone Oct 14, 2020
@kkangs0226 kkangs0226 added this to In progress in v1.2 (by 16/10/2020) via automation Oct 14, 2020
@kkangs0226 kkangs0226 self-assigned this Oct 14, 2020
@codecov-io
Copy link

codecov-io commented Oct 14, 2020

Codecov Report

Merging #53 into master will increase coverage by 2.24%.
The diff coverage is 97.26%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #53      +/-   ##
============================================
+ Coverage     72.52%   74.76%   +2.24%     
- Complexity      593      669      +76     
============================================
  Files            99      105       +6     
  Lines          1838     2021     +183     
  Branches        198      222      +24     
============================================
+ Hits           1333     1511     +178     
- Misses          433      435       +2     
- Partials         72       75       +3     
Impacted Files Coverage Δ Complexity Δ
src/main/java/seedu/momentum/model/Model.java 100.00% <ø> (ø) 1.00 <0.00> (ø)
...eedu/momentum/model/project/UniqueProjectList.java 91.13% <89.47%> (-1.55%) 33.00 <13.00> (+13.00) ⬇️
...in/java/seedu/momentum/model/project/Deadline.java 89.36% <93.75%> (+2.26%) 23.00 <9.00> (+9.00)
...rc/main/java/seedu/momentum/commons/core/Date.java 88.00% <100.00%> (+3.00%) 12.00 <3.00> (+3.00)
...ain/java/seedu/momentum/commons/core/DateTime.java 84.84% <100.00%> (+4.07%) 16.00 <3.00> (+3.00)
...rc/main/java/seedu/momentum/commons/core/Time.java 75.00% <100.00%> (+6.57%) 9.00 <3.00> (+3.00)
...ava/seedu/momentum/logic/commands/SortCommand.java 100.00% <100.00%> (ø) 15.00 <15.00> (?)
...in/java/seedu/momentum/logic/parser/CliSyntax.java 88.88% <100.00%> (+3.17%) 1.00 <0.00> (ø)
...seedu/momentum/logic/parser/ProjectBookParser.java 100.00% <100.00%> (ø) 15.00 <0.00> (+1.00)
...seedu/momentum/logic/parser/SortCommandParser.java 100.00% <100.00%> (ø) 14.00 <14.00> (?)
... and 14 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 5bede92...27371b3. Read the comment docs.

kkangs0226 and others added 5 commits October 14, 2020 14:07
User Guide does not have a section for sort command.

Updating user guide to include sort instructions would improve
user guide.

Let's update user guide to include sort instructions.
Accidentally removed a character from User Guide.

Let's fix the change.
Copy link
Collaborator

@pr4aveen pr4aveen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have a few comments that might need to be addressed before merging. Should be ok to merge once those changes have been made :)

src/main/java/seedu/momentum/model/ModelManager.java Outdated Show resolved Hide resolved
* and returns a SortCommand object for execution.
* @throws ParseException if the user does not conform to the expected format.
*/
public SortCommand parse(String args) throws ParseException {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just checking, what's the expected behaviour when there is a preamble?

For example, sort hello type/alpha order/dsc

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should disregard the preamble.
I have added a test for this!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have changed the SortCommandParser to fail when there is a non-empty preamble for consistency as AddCommandParser and FindCommandParser does the same. I have added a test accordingly!

Copy link
Collaborator

@khoodehui khoodehui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some things to take note, not too much of an issue though. Otherwise LGTM!

v1.2 (by 16/10/2020) automation moved this from In progress to Reviewer approved Oct 14, 2020
Copy link
Collaborator

@claracheong4 claracheong4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think some minor changes has to be made.

resolve comments

Name, Deadline, Date, DateTime, Time classes can inherit from
Comparable interface to make them comparable.
Some long methods can be abstracted further and more static
variables can be declared to make code more readable.

Let's
* implement compareTo methods in these classes
* update code to make it more readable
Wrong import order causes checkstyle error.

Let's update import order.
Test for parsing empty arguments fails on Windows.

Let's update SortCommandParserTest.
Test for parsing empty arguments fails on Windows.

Let's update SortCommandParserTest.
compareTo methods have not been tested.

Add tests to increase test code coverage.

Let's add tests for compareTo methods.
@kkangs0226 kkangs0226 merged commit edb3a2e into AY2021S1-CS2103T-T10-1:master Oct 14, 2020
v1.2 (by 16/10/2020) automation moved this from Reviewer approved to Done Oct 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority.Medium Nice to have
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

As a user, I can sort the projects in different orders Add Sort Feature
5 participants