Add Event reminders #135

bangyiwu · 2020-10-20T12:54:48Z

Allows setting reminders for events x days in advance

LinkedInk

Please reconsider the command structure used to be more consistent and edit all copy pasting references that are incorrect. The rest seems ok.

LinkedInk · 2020-10-24T06:50:02Z

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

-import seedu.address.logic.commands.IntroCommand;
-import seedu.address.logic.commands.ListContactCommand;
-import seedu.address.logic.commands.ListEventCommand;
+import seedu.address.logic.commands.*;


Mind the star import command causing checkstyle to fail.

LinkedInk · 2020-10-24T06:53:16Z

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

+    REMIND("remind"),
+    REMINDER("reminder"),


Confusing command names. Should change into a "add -r" and "show/list -r" style like how contacts and events are handled to keep consistency.

LinkedInk · 2020-10-24T06:56:09Z

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

+     * Parses the given {@code String} of arguments in the context of the AddContactCommand
+     * and returns an AddContactCommand object for execution.
+     * @throws ParseException if the user input does not conform the expected format


AddContactCommand references should not be here. Please double check if copy pasting has left anything that is not relevant and needs to be changed.

LinkedInk · 2020-10-28T15:08:31Z

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

+    REMIND("remind"),
+    REMINDER("listr"),
+    SHOW_REMINDER("showr"),


What I meant from a previous comment was to split it into command word and command type. Not to modify the command word.

So right now, contact commands have the command type "-c", event commands "-e", and tag commands "-t".

Reminder should have its own type, allowing for a reminder oriented set of commands. Reminder commands can have the command type "-r".

This means that things like the command word "add" can be reused (instead of "remind", and "list" instead of "listr"), combining it with the "-r" command type and it becomes "add -r" to signal an adding of a reminder command. This keeps the overall experiece relatively consistent with the rest of the commands as well.

LinkedInk

Command structure needs to be reconsidered, reminder commands should be grouped separtedly. Rest seems ok for now.

LinkedInk · 2020-10-28T15:16:28Z

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

+            case REMIND:
+                return new RemindEventCommandParser().parse(arguments);
+
+            case REMINDER:
+                return new ListReminderEventCommand();
+


Theses cases are ambigously named (REMIND vs REMINDER) when they clearly mean different things. They are also separated from the show reminder command when they should be grouped under another command type for reminder. Should not belong to events.

LinkedInk · 2020-10-28T15:18:28Z

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

+            case SHOW_REMINDER:
+                return new ShowReminderEventCommand();
+


This is separated from the 2 other reminder commands when they should be grouped under a single command type for reminder. Should not belong to the default branch.

LinkedInk · 2020-10-28T16:23:58Z

src/main/java/seedu/address/model/event/Reminder.java

+    private static String home = System.getProperty("user.dir");
+    private static Path savePath = Paths.get(home, "data", "reminders.txt");
+    private static ReminderStorage reminderStorage = new ReminderStorage(savePath);


Please use the UserPrefs class to get paths as standard. Helps when trying to create test classes as well, making it possible to mirror off other test classes that needs sample files (ie need to change directory/make a fake directory for testing).

LinkedInk · 2020-10-28T16:24:58Z

src/main/java/seedu/address/model/event/Reminder.java

+    static {
+        try {
+            reminders = reminderStorage.load();
+        } catch (IOException e) {
+            e.printStackTrace();
+        }
+    }


Not quite sure what this is. A constructor?

LinkedInk · 2020-10-28T16:30:55Z

src/main/java/seedu/address/storage/ReminderStorage.java

+import seedu.address.model.event.Reminder;
+import seedu.address.model.event.Time;
+
+public class ReminderStorage {


This class should be part of StorageManager as per addressbook, calendar, userPrefs, tagtree. Initial read should be part of MainApp init() method as show below.

It would be preferable to implement it in a .json file as well for saving and reading so that its standard. However, this is not a big deal as compared to just being a part of StorageManager. A .txt save file is fine as well.

LinkedInk · 2020-10-28T16:40:08Z

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

+    /**
+     * Executes the command to show the introduction. As the introduction command should not be
+     * accessible by the user, a commandText should not exist for it and the entire execution
+     * should be handled in the back-end. Note that as this method cannot throw a CommandException
+     * or a ParseException, handling can be ignored. The lack of an access modifier is intentional
+     * - method is supposed to be package private.
+     */
+
+    CommandResult executeShowReminderCommand() throws CommandException, ParseException {
+        CommandResult commandResult = logic.execute("showr");
+        logger.info("Result: " + commandResult.getFeedbackToUser());
+        resultPanel.setFeedbackToUser(commandResult.getFeedbackToUser());
+        handleReminders();
+        return commandResult;
+    }
+


If it can't throw, the method should not have a throws keyword. Any other method that uses this are forced to implement catching the exceptions or throwing them again. Which is not the same as "this method cannot throw a CommandException or a ParseException, handling can be ignored".

In the case where command word for show reminder is changed in the enum but not changed here, this method will throw a exception.

Do not follow executeIntroCommand() method above, its not a good implementation. Import enum CommandWord to avoid such incident. Implement catching and processing in case of an error, perhaps displaying an error message saying something like "reminders failed to show" in the resultPanel. Think defensive programming.

If it can't throw, the method should not have a throws keyword. Any other method that uses this are forced to implement catching the exceptions or throwing them again. Which is not the same as "this method cannot throw a CommandException or a ParseException, handling can be ignored".

you realise that any command that uses execute has to either catch and deal with a CommandException and ParseException or it has to throws it as well?

Also, it would be really helpful to say why something isn't a good implementation instead of brushing it aside as one.

this information is outright incorrect. the fact that you've said something like this shows that you do not understand how the method works.

If it can't throw, the method should not have a throws keyword.

correct.

Which is not the same as "this method cannot throw a CommandException or a ParseException, handling can be ignored".

incorrect. because line 229 uses Logic.execute(), your method has to either implement a way to catch these exceptions by embedding it in a try-catch block, or simply put it in the signature. the problem with the former is that your catch block then cannot be empty, as if the exception is thrown by Logic.execute(), neither is a CommandResult returned nor an exception thrown. the reason you can put it in the signature is because, as the back-end programmer, you know that it will never throw this exception.

basically, the method can throw it but won't.

In the case where command word for show reminder is changed in the enum but not changed here, this method will throw a exception.

yes, but CommandException and ParseException are both for users. as the programmer, you are expected to make consistent changes across the codebase when one reference changes. to use either of those exceptions as a way to ascertain that is wrong.

Import enum CommandWord to avoid such incident. Implement catching and processing in case of an error, perhaps displaying an error message saying something like "reminders failed to show" in the resultPanel.

this is literally what line 231 does.

Do not follow executeIntroCommand() method above, its not a good implementation.

would you like to explain why? if you want to say this, at least have the courtesy to @ me. if you want to discuss the implementation i'm all ears, but i'd prefer that to you straight up, condescendingly and accusatively stating that "its [sic] not a good implementation".

This is continued in person.

LinkedInk

Accidental extra review, ignore this.

First iteration of Event Reminders

6055e20

bangyiwu added this to the v1.3 milestone Oct 20, 2020

Create storage for reminders

f008af2

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

As a user, I should be able to set and view reminders #136

Closed

LinkedInk reviewed Oct 24, 2020

View reviewed changes

bangyiwu added 9 commits October 26, 2020 19:15

Update from master

004b432

Add PopUp window and countdown

8276e31

Refactor into correct packages

0af6683

Improve styling

9af2570

Improve styling 2

448e5ac

Improve styling 3

f684b41

Fix duplicate reminder bug

3d87247

Add javadoc

cd8594c

Fix naming

052f1f3

bangyiwu requested review from fyshhh, chan-j-d, LinkedInk and solkim-83 October 26, 2020 17:11

chan-j-d assigned bangyiwu Oct 27, 2020

chan-j-d added enhancement New feature or request priority.High Features of high priority for development. labels Oct 27, 2020

LinkedInk reviewed Oct 28, 2020

View reviewed changes

bangyiwu added 2 commits October 29, 2020 02:55

Refactor command type and word

0de7d8b

Resolve merge conflict

62d2b9c

bangyiwu added 3 commits October 29, 2020 03:02

Fix checkstyle

363dc08

Fix checkstyle 2

4f7882b

Fix checkstyle 3

0aeedc2

bangyiwu closed this Oct 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Event reminders #135

Add Event reminders #135

bangyiwu commented Oct 20, 2020

LinkedInk left a comment

LinkedInk Oct 24, 2020

LinkedInk Oct 24, 2020

LinkedInk Oct 24, 2020

LinkedInk Oct 28, 2020 •

edited

LinkedInk left a comment

LinkedInk Oct 28, 2020 •

edited

LinkedInk Oct 28, 2020 •

edited

LinkedInk Oct 28, 2020 •

edited

LinkedInk Oct 28, 2020

LinkedInk Oct 28, 2020 •

edited

LinkedInk Oct 28, 2020 •

edited

chan-j-d Oct 29, 2020 •

edited

chan-j-d Oct 29, 2020

fyshhh Oct 29, 2020 •

edited

LinkedInk Oct 29, 2020

LinkedInk left a comment •

edited

Add Event reminders #135

Add Event reminders #135

Conversation

bangyiwu commented Oct 20, 2020

LinkedInk left a comment

Choose a reason for hiding this comment

LinkedInk Oct 24, 2020

Choose a reason for hiding this comment

LinkedInk Oct 24, 2020

Choose a reason for hiding this comment

LinkedInk Oct 24, 2020

Choose a reason for hiding this comment

LinkedInk Oct 28, 2020 • edited

Choose a reason for hiding this comment

LinkedInk left a comment

Choose a reason for hiding this comment

LinkedInk Oct 28, 2020 • edited

Choose a reason for hiding this comment

LinkedInk Oct 28, 2020 • edited

Choose a reason for hiding this comment

LinkedInk Oct 28, 2020 • edited

Choose a reason for hiding this comment

LinkedInk Oct 28, 2020

Choose a reason for hiding this comment

LinkedInk Oct 28, 2020 • edited

Choose a reason for hiding this comment

LinkedInk Oct 28, 2020 • edited

Choose a reason for hiding this comment

chan-j-d Oct 29, 2020 • edited

Choose a reason for hiding this comment

chan-j-d Oct 29, 2020

Choose a reason for hiding this comment

fyshhh Oct 29, 2020 • edited

Choose a reason for hiding this comment

LinkedInk Oct 29, 2020

Choose a reason for hiding this comment

LinkedInk left a comment • edited

Choose a reason for hiding this comment

LinkedInk Oct 28, 2020 •

edited

LinkedInk Oct 28, 2020 •

edited

LinkedInk Oct 28, 2020 •

edited

LinkedInk Oct 28, 2020 •

edited

LinkedInk Oct 28, 2020 •

edited

LinkedInk Oct 28, 2020 •

edited

chan-j-d Oct 29, 2020 •

edited

fyshhh Oct 29, 2020 •

edited

LinkedInk left a comment •

edited