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

[Sien Soon] iP #505

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

[Sien Soon] iP #505

wants to merge 65 commits into from

Conversation

ssyap98
Copy link

@ssyap98 ssyap98 commented Aug 29, 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)

Copy link

@BlopApple BlopApple left a comment

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;

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 on lines 21 to 23
duke.greeting();
duke.run();
duke.goodBye();

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 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();
}

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?

Copy link

@TYKCodes TYKCodes left a comment

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

public class Duke {

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

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 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

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