Implements stats feature #114

jietung · 2019-10-24T04:51:08Z

No description provided.

# Conflicts: # src/main/java/seedu/exercise/logic/commands/AddExerciseCommand.java # src/main/java/seedu/exercise/logic/commands/DeleteExerciseCommand.java # src/main/java/seedu/exercise/logic/commands/EditCommand.java

# Conflicts: # src/main/java/seedu/exercise/logic/parser/ParserUtil.java

# Conflicts: # src/main/java/seedu/exercise/logic/Logic.java # src/main/java/seedu/exercise/logic/LogicManager.java # src/main/java/seedu/exercise/logic/parser/ExerciseBookParser.java # src/main/java/seedu/exercise/model/Model.java # src/main/java/seedu/exercise/model/ModelManager.java

…r stats command

t-cheepeng · 2019-10-24T10:02:47Z

src/main/java/seedu/exercise/logic/commands/DeleteExerciseCommand.java

+    /**
+     * Update statistic about deleted exercise.
+     */
+    private void updateStatistic(Model model) {


you can consider placing this method in the model class and just call model.updateStatistics().

t-cheepeng · 2019-10-24T10:08:42Z

src/main/java/seedu/exercise/logic/commands/statistic/StatsFactory.java

+        if (startDate == null && endDate == null) {
+            this.startDate = Date.getOneWeekBeforeToday();
+            this.endDate = Date.getToday();
+        } else {


Please document this null behaviour well. Include it in the documentation

t-cheepeng · 2019-10-24T10:09:35Z

src/main/java/seedu/exercise/logic/commands/statistic/StatsFactory.java

+     * Generates and returns statistic for different chart type.
+     */
+    public Statistic generateStatistic() {
+        HashMap<String, Double> data;


what is this hashmap for?

Deleted. I forgot to delete.

t-cheepeng · 2019-10-24T10:10:19Z

src/main/java/seedu/exercise/logic/commands/statistic/StatsFactory.java

+
+            return generatePieChartStatistic();
+
+        } else { //barchart


you may want to add else if (chart.equals("barchart")) and a else block that throws an exception

may want to consider using switch case to make it easier to read

t-cheepeng · 2019-10-24T10:11:44Z

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

+    public static final Prefix PREFIX_STARTDATE = new Prefix("s/");
+    public static final Prefix PREFIX_ENDDATE = new Prefix("e/");


START_DATE and END_DATE

t-cheepeng · 2019-10-24T10:15:47Z

src/main/java/seedu/exercise/model/property/Date.java

 import java.time.format.DateTimeFormatter;
 import java.time.format.DateTimeParseException;
+import java.util.ArrayList;

 /**
 * Represents an Exercise's date in ExerHealth.


Change the documentation here to reflect what the date now does. It is no longer just an Exercise's date

t-cheepeng · 2019-10-24T10:18:42Z

src/main/java/seedu/exercise/model/property/Date.java

+     * Returns today's Date.
+     */
+    public static Date getToday() {
+        LocalDate today = LocalDate.now(ZoneId.of("Asia/Singapore"));


you may want to use ZoneId.systemDefault() instead of hardcoding our zone

t-cheepeng · 2019-10-24T10:20:26Z

src/main/java/seedu/exercise/ui/BarChartPanel.java

+        barChart.setTitle(statistic.getCategory() + " (" + statistic.getStartDate()
+                + " to " + statistic.getEndDate() + ")");


May want to abstract this portion out into a Util class so all your xxxPanels can use it to format the title text.

t-cheepeng · 2019-10-24T10:26:52Z

src/test/java/seedu/exercise/testutil/StatisticBuilder.java

+    public StatisticBuilder() {
+        this.category = DEFAULT_CATEGORY;
+        this.chart = DEFAULT_CHART;
+        this.startDate = new Date(DEFAULT_START_DATE);
+        this.endDate = new Date(DEFAULT_END_DATE);
+        this.properties = new ArrayList<>();
+        this.values = new ArrayList<>();
+    }
+
+    public Statistic build() {
+        return new Statistic(category, chart, startDate, endDate, properties, values);


This is not how a builder would look like. This is more of a get a default statistics. Look at #96 under CustomPropertyBuilder to see how it would look like

t-cheepeng · 2019-10-24T10:42:43Z

src/main/java/seedu/exercise/logic/commands/statistic/StatsFactory.java

+    /**
+     * Compute exercise count with filtered exercises list.
+     */
+    private HashMap<String, Double> getTotalExerciseCount() {


Maybe you can just return a list of double and abstract out the methods. The getTotalExerciseCount() is very similar to getTotalExerciseQuantity(). Entirely up to you if you wish to change

To minimize confusion between this method and the one below, can consider calling this getTotalExerciseFrequency()

Changed to getTotalExerciseFrequency()

garylyp · 2019-10-24T16:58:13Z

src/main/java/seedu/exercise/logic/Logic.java

@@ -79,6 +80,11 @@
     */
    void setGuiSettings(GuiSettings guiSettings);

+    /**
+     * Returns the Statistic.
+     */


Can be slightly more descriptive about this comment. Maybe "Returns the Statistic object currently in focus" or something that describe its purpose.

garylyp · 2019-10-24T17:05:25Z

src/main/java/seedu/exercise/logic/commands/statistic/StatsCommand.java

+        StatsFactory statsFactory = new StatsFactory(exercises, chart, category, startDate, endDate);
+        Statistic statistic = statsFactory.generateStatistic();
+        model.updateStatistic(statistic);
+        return new CommandResult("Chart displayed");


Can consider replacing this magic literal with a MESSAGE_STATS_DISPLAY_SUCCESS

garylyp · 2019-10-24T17:59:34Z

src/main/java/seedu/exercise/logic/commands/statistic/StatsFactory.java

+    /**
+     * Compute exercise count with filtered exercises list.
+     */
+    private HashMap<String, Double> getTotalExerciseCount() {


To minimize confusion between this method and the one below, can consider calling this getTotalExerciseFrequency()

garylyp · 2019-10-24T18:06:43Z

src/main/java/seedu/exercise/logic/parser/StatsCommandParser.java

+            int numberOfDaysApart = Date.numberOfDaysBetween(startDate, endDate);
+
+            if (numberOfDaysApart > 31) {
+                throw new ParseException("Start date and end date are too far apart");


Might want to specify what's the max difference in the error messages. Also, better to store the error messages in a static final String. Same for below

garylyp · 2019-10-24T18:07:42Z

src/main/java/seedu/exercise/logic/parser/StatsCommandParser.java

+        } else if (argMultimap.arePrefixesPresent(PREFIX_ENDDATE)
+                && !argMultimap.arePrefixesPresent(PREFIX_STARTDATE)) { //only end date present
+
+            throw new ParseException("Start date must be provided.");


Can simply use "Start date must be provided with end date" for both

garylyp · 2019-10-24T18:15:30Z

src/main/java/seedu/exercise/ui/MainWindow.java

+        } else if (chart.equals("linechart")) {
+            chartPlaceholder.getChildren().add(new LineChartPanel(statistic).getRoot());
+        } else {
+            chartPlaceholder.getChildren().add(new PieChartPanel(statistic).getRoot());


else if (chart.equals("piechart")) {

} else {
// Default chart / some text to show chart display error
}

kwekke · 2019-10-24T18:04:01Z

src/main/java/seedu/exercise/logic/commands/AddExerciseCommand.java

+        ReadOnlyResourceBook<Exercise> exercises = model.getExerciseBookData();
+        Statistic outdatedStatistic = model.getStatistic();
+        StatsFactory statsFactory = new StatsFactory(exercises, outdatedStatistic.getChart(),
+                outdatedStatistic.getCategory(), outdatedStatistic.getStartDate(), outdatedStatistic.getEndDate());


May want to implement a constructor in StatsFactory that takes in exercises and an outdatedStatistic object so that you don't violate Law of Demeter.

I have moved this method to model.

kwekke · 2019-10-24T18:08:21Z

src/main/java/seedu/exercise/logic/commands/statistic/StatsCommand.java

+    private String chart;
+    private String category;
+    private Date startDate;
+    private Date endDate;


May want to implement immutable variables instead

kwekke · 2019-10-24T18:10:02Z

src/main/java/seedu/exercise/logic/commands/statistic/StatsFactory.java

+
+            return generatePieChartStatistic();
+
+        } else { //barchart


may want to consider using switch case to make it easier to read

kwekke · 2019-10-24T18:29:02Z

src/main/java/seedu/exercise/logic/parser/ParserUtil.java

+        String trimmedEndDate = endDate.trim();
+        parseDate(trimmedEndDate);
+        if (!Date.isEndDateAfterStartDate(startDate.toString(), trimmedEndDate)) {
+            throw new ParseException("End date must be before ");


ParseException should be "End date must be before start date"?

kwekke · 2019-10-24T18:38:30Z

src/main/java/seedu/exercise/model/property/Date.java

+            if (eDate.compareTo(sDate) < 0) {
+                return false;
+            } else {
+                return true;


can simplify this expression to:
return eDate.compareTo(sDate) >= 0

kwekke · 2019-10-24T18:39:59Z

src/main/java/seedu/exercise/model/property/Date.java

+
+        if (d.compareTo(sDate) >= 0 && d.compareTo(eDate) <= 0) {
+            return true;
+        }


can simplify this expression as well

kwekke · 2019-10-24T19:02:32Z

src/main/java/seedu/exercise/logic/parser/StatsCommandParser.java

+
+            throw new ParseException("Start date must be provided.");
+
+        }


what if both start date and end date are not present?

it will set start day as a week before today's date and end date as today's date

kwekke · 2019-10-24T19:07:59Z

src/main/java/seedu/exercise/logic/commands/statistic/StatsFactory.java

+        ArrayList<Double> values = new ArrayList<>();
+
+        for (String n : names) {
+            values.add(data.get(n));


can be expressed as:
return new ArrayList<>(data.values())

I need it to correspond to the list of strings, so i did it this way.

kwekke · 2019-10-24T19:12:09Z

src/main/java/seedu/exercise/logic/commands/statistic/StatsFactory.java

+    /**
+     * Compute exercise quantity with filtered exercises list.
+     */
+    private HashMap<String, Double> getTotalExerciseQuantity() {


Naming of this method (and getTotalExerciseCount) can be misleading.

i have changed it to getTotalExerciseFrequency()

kwekke · 2019-10-24T19:14:51Z

src/main/java/seedu/exercise/logic/commands/statistic/StatsCommand.java

+        StatsFactory statsFactory = new StatsFactory(exercises, chart, category, startDate, endDate);
+        Statistic statistic = statsFactory.generateStatistic();
+        model.updateStatistic(statistic);
+        return new CommandResult("Chart displayed");


May want to store the message in CommandResult as a private final String MESSAGE_SUCCESS in this class.

# Conflicts: # src/main/java/seedu/exercise/logic/parser/CliSyntax.java # src/main/java/seedu/exercise/logic/parser/ParserUtil.java # src/main/java/seedu/exercise/model/Model.java # src/main/java/seedu/exercise/model/ModelManager.java # src/test/java/seedu/exercise/logic/commands/AddExerciseCommandTest.java

garylyp · 2019-10-25T15:11:47Z

src/main/java/seedu/exercise/logic/parser/ParserUtil.java

@@ -143,7 +147,7 @@ public static Date parseEndDate(Date startDate, String endDate) throws ParseExce
        String trimmedEndDate = endDate.trim();
        parseDate(trimmedEndDate);
        if (!Date.isEndDateAfterStartDate(startDate.toString(), trimmedEndDate)) {
-            throw new ParseException("End date must be before ");
+            throw new ParseException("End date must be after start date");


Would be better if the message is stored in a final String MESSAGE_INVALID_END_DATE in the ParserUtil class or Date class

…atistic

t-cheepeng

👍

# Conflicts: # src/main/java/seedu/exercise/logic/parser/CliSyntax.java # src/main/java/seedu/exercise/model/Model.java

jietung added 19 commits October 21, 2019 20:27

Add StatsCommandParser

8e8b85c

Add StatsCommand

294c0dc

Add methods in ParseUtil

970a84b

Add prefix in CliSyntax

eda80c9

Add StatsFactory

8b28d1d

Add Statistic

b96a2a6

Add default statistic

aa4114a

Add BarChartPanel

8a8fb14

Add LineChartPanel

24d8594

Add PieChartPanel

7dbc682

Refactor code

1972b2b

Add javadoc and refactor code

ff3e256

Add update statistic when model is edited

3564cb3

Merge branch 'master' into stats

e0bbb5c

# Conflicts: # src/main/java/seedu/exercise/logic/commands/AddExerciseCommand.java # src/main/java/seedu/exercise/logic/commands/DeleteExerciseCommand.java # src/main/java/seedu/exercise/logic/commands/EditCommand.java

Merge branch 'master' into stats

48787f4

# Conflicts: # src/main/java/seedu/exercise/logic/parser/ParserUtil.java

Refactor code for statistic in model

d21a9f5

Add statistic methods in modelstub in test

7f08e47

Edit values return for barchart

d7fe210

Add javadoc comments

5c84bc7

jietung requested review from t-cheepeng, garylyp, kwekke and weihaw08 October 24, 2019 04:51

Add newline at EOF in fxml

e09f656

jietung added this to the v1.3 milestone Oct 24, 2019

jietung added 2 commits October 24, 2019 17:02

Add restrictions to number of days between start date and end date fo…

5d3ba34

…r stats command

t-cheepeng reviewed Oct 24, 2019

View reviewed changes

garylyp reviewed Oct 24, 2019

View reviewed changes

kwekke reviewed Oct 24, 2019

View reviewed changes

jietung added 4 commits October 25, 2019 19:28

Refactor code

f3acd64

Update user guide

a7109bc

Add newline at EOF in data/exercisedatabases.json

7f8bd44

garylyp reviewed Oct 25, 2019

View reviewed changes

t-cheepeng requested review from t-cheepeng and kwekke October 25, 2019 15:32

Change strings in ParserUtil and store as final String in Date and St…

6b52ef9

…atistic

t-cheepeng approved these changes Oct 26, 2019

View reviewed changes

Merge branch 'master' into stats

84b5db1

# Conflicts: # src/main/java/seedu/exercise/logic/parser/CliSyntax.java # src/main/java/seedu/exercise/model/Model.java

kwekke approved these changes Oct 26, 2019

View reviewed changes

t-cheepeng merged commit 780bbaa into AY1920S1-CS2103T-T09-2:master Oct 26, 2019

		public static final Prefix PREFIX_STARTDATE = new Prefix("s/");
		public static final Prefix PREFIX_ENDDATE = new Prefix("e/");

		barChart.setTitle(statistic.getCategory() + " (" + statistic.getStartDate()
		+ " to " + statistic.getEndDate() + ")");

Implements stats feature #114

Implements stats feature #114

Conversation

jietung commented Oct 24, 2019

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

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

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

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

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

t-cheepeng left a comment

Choose a reason for hiding this comment