Skip to content

[Sien Soon] iP - #505

Open
ssyap98 wants to merge 65 commits into
nus-cs2103-AY2223S1:masterfrom
ssyap98:master
Open

[Sien Soon] iP#505
ssyap98 wants to merge 65 commits into
nus-cs2103-AY2223S1:masterfrom
ssyap98:master

Conversation

@ssyap98

@ssyap98 ssyap98 commented Aug 29, 2022

Copy link
Copy Markdown

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)

@BlopApple BlopApple left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Overall your code looks fine, although could probably use a bit more abstraction for future implementations.


public class Deadline extends Task {

protected LocalDateTime by;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Maybe you could name it as "deadlineTime" instead of "by"?

Comment thread src/main/java/Duke.java Outdated
Comment on lines +21 to +23
duke.greeting();
duke.run();
duke.goodBye();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Maybe you could name the methods as verbs like greet() and exit() instead?

Comment thread src/main/java/Duke.java Outdated
Comment on lines +44 to +67
case "T": {
Task task = new ToDo(taskInArray[2]);
taskList.add(task);
if (taskInArray[1].equals("1")) {
task.markAsDone();
}
break;
}

case "D": {
Task task = new Deadline(taskInArray[2], taskInArray[3]);
taskList.add(task);
if (taskInArray[1].equals("1")) {
task.markAsDone();
}
break;
}

case "E": {
Task task = new Event(taskInArray[2], taskInArray[3]);
taskList.add(task);
if (taskInArray[1].equals("1")) {
task.markAsDone();
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Since in each case you are marking the task as done when loading them, maybe u can abstract the process out?

@TYKCodes TYKCodes left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Overall, the code looks clean and understandable. Just a few touching up of the formatting and it should be all good

Comment thread src/main/java/Duke.java Outdated

public class Duke {

private final String horizontalLine = "-------------------------";

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You may want the name of your constant variables like this one to match the coding standard. Perhaps something like:

Suggested change
private final String horizontalLine = "-------------------------";
private final String HORIZONTAL_LINE = "-------------------------";

Comment thread src/main/java/Duke.java Outdated
Comment on lines +43 to +69
switch (taskType) {
case "T": {
Task task = new ToDo(taskInArray[2]);
taskList.add(task);
if (taskInArray[1].equals("1")) {
task.markAsDone();
}
break;
}

case "D": {
Task task = new Deadline(taskInArray[2], taskInArray[3]);
taskList.add(task);
if (taskInArray[1].equals("1")) {
task.markAsDone();
}
break;
}

case "E": {
Task task = new Event(taskInArray[2], taskInArray[3]);
taskList.add(task);
if (taskInArray[1].equals("1")) {
task.markAsDone();
}
break;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The indentation for each case statement should be aligned to the switch

Exits Duke when "bye" is entered by user
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