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

[Silas Yeo] iP #485

Open
wants to merge 43 commits into
base: master
Choose a base branch
from

Conversation

comicalromance
Copy link

@comicalromance comicalromance commented Aug 27, 2022

DukePro

“Your mind is for having ideas, not holding them.” – David Allen (source)

DukePro frees your mind of having to remember things you need to do. It's,

  • text-based
  • easy to learn
  • FAST SUPER FAST to use

All you need to do is,

  1. download it from here.
  2. double-click it.
  3. add your tasks.
  4. let it manage your tasks for you 🐱

And it is FREE!

Features:

  • Managing tasks
  • Managing deadlines (coming soon)
  • Reminders (coming soon)

If you Java programmer, you can use it to practice Java too. Here's the main method:

public class Duke {
    public static void main(String[] args) {
        new Duke(FILE_LOCATION, FOLDER_LOCATION).run();
    }
}

Copy link

@tingkai-mai tingkai-mai left a comment

Choose a reason for hiding this comment

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

LGTM! I think I would suggest renaming some of the enum values to make it clearer, and perhaps throw/catch exceptions using DukeException rather than Exception.

src/main/java/duke/command/Command.java Outdated Show resolved Hide resolved
src/main/java/duke/command/DeleteCommand.java Outdated Show resolved Hide resolved
src/main/java/duke/task/Task.java Show resolved Hide resolved
src/main/java/duke/task/Task.java Show resolved Hide resolved
src/main/java/duke/Parser.java Show resolved Hide resolved
src/main/java/duke/Storage.java Outdated Show resolved Hide resolved
Made modifications to several access modifiers, method names, enum values and exceptions returned.
Copy link

@rexong rexong left a comment

Choose a reason for hiding this comment

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

Your code looks good. Most of it is following the coding standard! Just some of the minor suggestions regarding your method and attributes naming.

@FXML
private Label dialog;
@FXML
private ImageView displayPicture;
Copy link

Choose a reason for hiding this comment

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

Perhap it is better to name is as a noun? example pictureDiplayer.

this(FILE_LOCATION, FOLDER_LOCATION);
}

public boolean getLoaded() {
Copy link

Choose a reason for hiding this comment

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

Maybe the method name can be changed to isLoaded instead?

Comment on lines +3 to +11
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.api.Assertions.fail;

import org.junit.jupiter.api.Test;

import duke.command.EventCommand;
import duke.command.ExitCommand;
import duke.command.MarkCommand;
import duke.command.UnmarkCommand;
Copy link

Choose a reason for hiding this comment

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

I like how your import statements are in order. 👍

Copy link

@tanhl2000 tanhl2000 left a comment

Choose a reason for hiding this comment

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

Overall great job with the code! Coding standards mostly followed as well.

if (duke.getLoaded()) {
print("File successfully loaded!");
} else {
print("Error parsing load file");

Choose a reason for hiding this comment

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

This can be put under Exception handling instead as it is an error message that should show with an exception. DukeException could also be expanded upon to have multiple more specific exceptions.

mainClassName in the Gradle config is set to duke.Duke.

The current main class should be duke.Launcher instead.

The variable has thus been changed to duke.Launcher.
Some methods were missing assertions that could be useful for
developers.

Assertions could be used to make sure unexpected behaviour does not
occur during runtime.

Let's add assert statements to two segments of code:
* In Parser, an assert in getTaskName was added to ensure the input
array has more than one element, indicating a name is present
* In  TaskList, an assert in the constructor was added to ensure that
if the task parsed is a ToDo (msg.length == 1), that no date has been
parsed as expected.
Parser method is extensive with numerous duplicate code fragments.

Excessively long code is difficult to both read and debug.

Let's refactor code by extracting two methods:
* create parseDateTime to convert string into LocalDateTime
* create tokenizeInput to convert string into tokens with error
handling
There is no simple way to organize tasks in the task list.

Users who want to sort their tasks by upcoming deadline or alphabetical
order could not do so easily.

Let's add a sort command that allows users to sort the list either
alphabetically or chronologically, and in ascending or descending
order.

Let's also refactor the Task class to include time as well.
Let's add more JavaDoc comments to previously undocumented code.
Sort command only parses user input if it strictly matches the listed parameters.
Users need to type entire parameter, which is long and not user-friendly.

Let's check if user input matches the starting characters of the parameter instead of the whole string.
Error messages are undifferentiated from normal Duke messages.
Users may be unsure when they are getting error messages.

Let's add an error message dialog to better distinguish the two.
Let's update the user guide for a more user-friendly experience.
Remove duplicate entry for list in user guide.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants