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

[Rebecca Lau] iP #476

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

Conversation

rebeccalaujx
Copy link

@rebeccalaujx rebeccalaujx commented Aug 26, 2021

Duke

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

Duke 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
  • Reminders (coming soon)

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

    public static void main(String[] args) {
        new Duke("duke.txt").run();
    }

@rebeccalaujx rebeccalaujx changed the title [{Rebecca Lau}] iP [Rebecca Lau] iP Aug 26, 2021
Copy link

@kleonang kleonang left a comment

Choose a reason for hiding this comment

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

Hi there, I've looked through your code and your code quality is generally good with some minor improvements to be made 😊.

Good use of OOP is evident, especially through the abstract Task and Command classes. You may also consider adding JavaDocs to the public constructors and methods to ease readability.

Great work on the code overall! 👍

@@ -0,0 +1,25 @@
package duke;

import duke.command.*;
Copy link

Choose a reason for hiding this comment

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

Would be better to specifically list all duke.command imports here instead of using wildcard imports

Comment on lines 11 to 20
} else if (input.startsWith("done")) {
return new DoneCommand(input);
} else if (input.startsWith("delete")) {
return new DeleteCommand(input);
} else if (input.startsWith("todo")) {
return new TodoCommand(input);
} else if (input.startsWith("deadline")) {
return new DeadlineCommand(input);
} else if (input.startsWith("event")) {
return new EventCommand(input);
Copy link

Choose a reason for hiding this comment

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

Using input.startsWith("...") may cause some invalid commands to be interpreted as valid commands, e.g. when input starts with deadliner or todoy. Instead, consider using input.split(" ", 2) to get a String[], then check equality against the first element of the String[]. This way, the second element of the String[] also contains all the information you need for your Command subclasses later on.

@@ -0,0 +1,96 @@
package duke;

import duke.task.*;
Copy link

Choose a reason for hiding this comment

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

Would be better to specifically list imports as before

Comment on lines 13 to 15
String filePath;
PrintWriter writer;
TaskList ls;
Copy link

Choose a reason for hiding this comment

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

Would be good to add private access modifiers here

Comment on lines +26 to +30
} catch (FileNotFoundException e) {
e.printStackTrace();
} catch (IOException e) {
e.printStackTrace();
}
Copy link

Choose a reason for hiding this comment

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

Possible to combine into a single catch statement here

Suggested change
} catch (FileNotFoundException e) {
e.printStackTrace();
} catch (IOException e) {
e.printStackTrace();
}
} catch (FileNotFoundException | IOException e) {
e.printStackTrace();
}


public abstract class Command {
public abstract void execute(TaskList ls, Ui ui, Storage storage) throws DukeException;
public abstract boolean isExit();
Copy link

Choose a reason for hiding this comment

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

Since all but one of the commands have isExit() as false, this can be made into a non-abstract method.

Suggested change
public abstract boolean isExit();
public boolean isExit() {
return false;
}

Comment on lines +29 to +32
@Override
public boolean isExit() {
return false;
}
Copy link

Choose a reason for hiding this comment

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

After the non-abstract isExit() is implemented in Command.java, there's no need to override here (and all commands except ExitCommand.java) anymore.

Comment on lines +14 to +17
@Override
public boolean isExit() {
return true;
}
Copy link

Choose a reason for hiding this comment

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

Here should be the only Command subclass where overriding the isExit() method is necessary after implementing it in Command.java.

Comment on lines 23 to 30
if (description.isEmpty() || description == "" || description == " ") {
throw new DukeException("☹ OOPS!!! The description of a deadline cannot be empty.");
} else {
this.description = description.substring(1);
}

if (deadline.isEmpty() || deadline == "" || deadline == " ") {
throw new DukeException("☹ OOPS!!! The deadline of this task must be indicated.");
Copy link

Choose a reason for hiding this comment

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

As before, these repeated checks in the if statements should not be necessary after using input.split(" ", 2) in Parser.java.

Comment on lines 23 to 30
if (description.isEmpty() || description == "" || description == " ") {
throw new DukeException("☹ OOPS!!! The description of an event cannot be empty.");
} else {
this.description = description.substring(1);
}

if (at.isEmpty() || at == "" || at == " ") {
throw new DukeException("☹ OOPS!!! The time of the event must be indicated.");
Copy link

Choose a reason for hiding this comment

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

As before, these repeated checks in the if statements should not be necessary after using input.split(" ", 2) in Parser.java.

Copy link

@HangZelin HangZelin left a comment

Choose a reason for hiding this comment

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

Overall a pretty nice job for coding standard! And your OOP design is pretty nice too. Just to remind there can be some improvements for your import and packaging naming.

Keep on your good job! 👍

@@ -1,3 +1,7 @@
package duke;

import duke.command.*;

Choose a reason for hiding this comment

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

Is it better to not use wildcard for the import? It might be better to list all the stuff you want to import.

@@ -1,9 +1,12 @@
package duke;

import duke.task.*;

Choose a reason for hiding this comment

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

It might be better to list all the stuff you want to import instead of using wildcard.

@@ -15,7 +20,7 @@ public void showWelcome() {
+ "| |_| | |_| | < __/\n"
+ "|____/ \\__,_|_|\\_\\___|\n";
System.out.println("Hello from\n" + logo);
System.out.println("Hello! I'm Duke\n" + "What can I do for you?");
System.out.println("Hello! I'm duke.Duke\n" + "What can I do for you?");

Choose a reason for hiding this comment

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

You may import the duke package inside, and you not really have to specify package all the time.

@@ -14,7 +17,7 @@ public Deadline(String description, String deadline) throws DukeException {
LocalDate localDate = LocalDate.parse(deadline);
this.date = localDate.format(DateTimeFormatter.ofPattern("MMM dd yyyy"));
} catch (DateTimeParseException e) {
throw new DukeException("Deadline should be in a yyyy-mm-dd format.");
throw new DukeException("duke.task.Deadline should be in a yyyy-mm-dd format.");

Choose a reason for hiding this comment

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

You may import duke.task here.

rebeccalaujx and others added 21 commits September 16, 2021 13:39
The assertions feature addresses important assumptions that should
hold in the code.

A program that terminates the program early once a bug is observed
is safer to use as it prevents the error from escalating further.

Let's include the assertions feature to define assumptions about the
program state so that the runtime can verify them.
Information extraction from input was largely repeated throughout
the code, creating long methods and convoluted expressions.

Extracting common behaviour into the common Input class reduces the
amount of arrowhead style code, makes the expressions easier to
understand, and shortens long methods, hence improving the code's
readability.

Let's improve the code quality so that the code can be more readable.
Users might want to delete many tasks or set many tasks as done
all in one go, instead of entering a new command line for each task.

MassOps allows users to delete multiple tasks or set multiple tasks
as done, all in one go.

Let's update the functions that delete tasks and set tasks as done
to allow them to perform their function on multiple tasks at once.
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.

3 participants