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

[Chen Shun] IP #491

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

Conversation

ciaoosuuu
Copy link

@ciaoosuuu ciaoosuuu 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)

Copy link

@LianGuoYang LianGuoYang left a comment

Choose a reason for hiding this comment

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

LGTM. Just minor things to fix.

private String line = "___________________________________________________";
private String exit = "Bye. Hope to see you again soon!";
Scanner sc = new Scanner(System.in);
ArrayList<Task> tasks;

Choose a reason for hiding this comment

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

Should tasks be private instead of public ?

}
}

}

Choose a reason for hiding this comment

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

I like where you have multiple exceptions to account for different errors.

}

private String getStatusIcon() {
return (isDone ? "[X]" : "[ ]"); // mark done task with X

Choose a reason for hiding this comment

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

Perhaps you can move the comment to the top.

return this.getStatusIcon() + " " + this.description;
}

//...

Choose a reason for hiding this comment

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

Should this be removed?

@@ -0,0 +1,12 @@
public class ToDo extends Task {

Choose a reason for hiding this comment

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

Extra line break here, should this be removed?

} else {
System.out.println("Here are the tasks in your list:");
for (int i = 0; i < this.tasks.size(); i++) {
int sn = i + 1;

Choose a reason for hiding this comment

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

Maybe a intuitive name for variable sn?

myPony.initialise();
myPony.run();
}
}

Choose a reason for hiding this comment

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

I like where you throw exceptions for each commands and how you added comments for each command which made reading your code easier.

@@ -0,0 +1,195 @@
import java.util.Scanner;
import java.util.ArrayList;
public class Pony {

Choose a reason for hiding this comment

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

Perhaps a line break to separate line 2 and line 3?

@@ -0,0 +1,33 @@
import java.lang.RuntimeException;
public class PonyException {

Choose a reason for hiding this comment

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

Maybe a line break to separate both lines?

Copy link

@april-anh april-anh left a comment

Choose a reason for hiding this comment

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

Detailed comments make your code very easy to understand.

run();
}
} else if (action.equals("todo")) {
// tod0 expects task description in command[1]

Choose a reason for hiding this comment

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

You can use "to-do" to prevent the text to be marked green.

private void processCommand(String[] command) {
int commandSize = command.length;
String action = command[0];
if (action.equals("list")) {

Choose a reason for hiding this comment

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

Maybe using switch instead of if-else can make your code neater?

} catch (NumberFormatException e) {
System.out.println(new PonyException.taskInputError().getMessage());
} finally {
run();

Choose a reason for hiding this comment

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

I like how you carefully clean up your program after handling exceptions. 👍

Copy link

@tensaida tensaida left a comment

Choose a reason for hiding this comment

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

Good job! All the best for the upcoming weeks

}

//Task to mark not provided
public static class taskMissingError extends RuntimeException {
Copy link

Choose a reason for hiding this comment

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

You could make these Errors extend PonyException instead

}

public static void main(String[] args) {
Pony myPony = new Pony();
Copy link

Choose a reason for hiding this comment

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

Since only one Pony object is created, why not make everything static instead?

run();
}
} else if (action.equals("unmark")) {
// unmark expects input in command[1] -> which task to unmark
Copy link

Choose a reason for hiding this comment

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

good comment 👍🏾

ciaoosuuu and others added 16 commits September 19, 2022 17:55
Response from pony is not friendly to user.

Change some of the pony's response to a more friendly tone.
Find command is case sensitive.

A case insensitive find is more user-friendly because users cannot be
expected to remember the exact case of the keywords.

Let's update the search algorithm by converting all
strings to lowercase and find matches in lowercase.
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.

4 participants