-
Notifications
You must be signed in to change notification settings - Fork 462
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
base: master
Are you sure you want to change the base?
[Chen Shun] IP #491
Conversation
There was a problem hiding this 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.
src/main/java/Pony.java
Outdated
private String line = "___________________________________________________"; | ||
private String exit = "Bye. Hope to see you again soon!"; | ||
Scanner sc = new Scanner(System.in); | ||
ArrayList<Task> tasks; |
There was a problem hiding this comment.
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 ?
src/main/java/PonyException.java
Outdated
} | ||
} | ||
|
||
} |
There was a problem hiding this comment.
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.
src/main/java/Task.java
Outdated
} | ||
|
||
private String getStatusIcon() { | ||
return (isDone ? "[X]" : "[ ]"); // mark done task with X |
There was a problem hiding this comment.
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.
src/main/java/Task.java
Outdated
return this.getStatusIcon() + " " + this.description; | ||
} | ||
|
||
//... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be removed?
src/main/java/ToDo.java
Outdated
@@ -0,0 +1,12 @@ | |||
public class ToDo extends Task { | |||
|
There was a problem hiding this comment.
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?
src/main/java/Pony.java
Outdated
} else { | ||
System.out.println("Here are the tasks in your list:"); | ||
for (int i = 0; i < this.tasks.size(); i++) { | ||
int sn = i + 1; |
There was a problem hiding this comment.
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?
src/main/java/Pony.java
Outdated
myPony.initialise(); | ||
myPony.run(); | ||
} | ||
} |
There was a problem hiding this comment.
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.
src/main/java/Pony.java
Outdated
@@ -0,0 +1,195 @@ | |||
import java.util.Scanner; | |||
import java.util.ArrayList; | |||
public class Pony { |
There was a problem hiding this comment.
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?
src/main/java/PonyException.java
Outdated
@@ -0,0 +1,33 @@ | |||
import java.lang.RuntimeException; | |||
public class PonyException { |
There was a problem hiding this comment.
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?
There was a problem hiding this 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.
src/main/java/Pony.java
Outdated
run(); | ||
} | ||
} else if (action.equals("todo")) { | ||
// tod0 expects task description in command[1] |
There was a problem hiding this comment.
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.
src/main/java/Pony.java
Outdated
private void processCommand(String[] command) { | ||
int commandSize = command.length; | ||
String action = command[0]; | ||
if (action.equals("list")) { |
There was a problem hiding this comment.
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?
src/main/java/Pony.java
Outdated
} catch (NumberFormatException e) { | ||
System.out.println(new PonyException.taskInputError().getMessage()); | ||
} finally { | ||
run(); |
There was a problem hiding this comment.
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. 👍
There was a problem hiding this 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
src/main/java/PonyException.java
Outdated
} | ||
|
||
//Task to mark not provided | ||
public static class taskMissingError extends RuntimeException { |
There was a problem hiding this comment.
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
src/main/java/Pony.java
Outdated
} | ||
|
||
public static void main(String[] args) { | ||
Pony myPony = new Pony(); |
There was a problem hiding this comment.
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?
src/main/java/Pony.java
Outdated
run(); | ||
} | ||
} else if (action.equals("unmark")) { | ||
// unmark expects input in command[1] -> which task to unmark |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good comment 👍🏾
Add assertions
Improve code quality
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.
DukePro
DukePro frees your mind of having to remember things you need to do. It's,
FASTSUPER FAST to useAll you need to do is,
And it is FREE!
Features: