-
Notifications
You must be signed in to change notification settings - Fork 103
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
[Ho Wei Yi Stanley] iP #99
base: master
Are you sure you want to change the base?
Conversation
src/main/java/Task.java
Outdated
} | ||
|
||
public String getStatusIcon() { | ||
return (isDone ? "\u2713" : " "); //return tick symbol if task is done |
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 this unicode can be replaced with a more expressive constant instead, so that the comment can be removed
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.
Got it! Fixed :)
src/main/java/Duke.java
Outdated
System.out.println("\t__________________________________________\n"); | ||
|
||
ArrayList<Task> tasks = new ArrayList<Task>(); | ||
int taskCount = 0; |
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.
Instead of using the taskCount
variable, is it possible to use tasks.length
instead, so there is one less variable to manage?
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 idea. Fixed it already!
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.
Generally, no coding standard violations. Good job!
src/main/java/Deadline.java
Outdated
this.by = by; | ||
} | ||
|
||
@Override |
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.
Neat use of method overriding too for each entity
src/main/java/Duke.java
Outdated
// sets a task as done | ||
System.out.println("\tGreat job! I've marked this task as done: "); | ||
Task task = tasks.get(taskIndex - 1); | ||
task.markAsDone(); |
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.
All method names are in camelCase too
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.
Great job overall!
src/main/java/Duke.java
Outdated
System.out.println("\t__________________________________________\n"); | ||
|
||
ArrayList<Task> tasks = new ArrayList<>(); | ||
boolean runLoop = true; |
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.
Naming of boolean variable is very readable. Very good!
src/main/java/Event.java
Outdated
this.at= at; | ||
} | ||
|
||
@Override |
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 use of method overriding, allows Event class to inherit from parent Task class. Very good!
src/main/java/Duke.java
Outdated
// if list is empty | ||
System.out.println("\tYour list is empty!"); | ||
} | ||
else { |
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.
If else statements should follow proper code standards! Small issue as the other statements follow them.
src/main/java/Duke.java
Outdated
System.out.println("\tWhat can I help you with?"); | ||
System.out.println("\t__________________________________________\n"); | ||
|
||
ArrayList<Task> tasks = new ArrayList<>(); |
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.
Collection of tasks is in plural form! Good!
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.
Overall good code quality! Clean and easy to understand and track!
src/main/java/Duke.java
Outdated
while (runLoop) { | ||
|
||
String userInput = sc.nextLine().trim(); | ||
|
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.
No need to leave space between these
src/main/java/Task.java
Outdated
|
||
public Task(String description) { | ||
this.description = description; | ||
this.isDone = false; |
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 use of naming and Class with super
src/main/java/Deadline.java
Outdated
|
||
@Override | ||
public String toString() { | ||
return "[D]" + super.toString() + "(by: " + this.by + ")"; |
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.
This is a very clear structure of code, I will also apply this in mine
src/main/java/Duke.java
Outdated
System.out.println("\t__________________________________________\n"); | ||
} else { | ||
// adds a task to the list | ||
System.out.println("\t------------------------------------------"); |
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 the usage of a different form of lines, one:----- and one––––––––, it performs different meanings and let users easily understood.
src/main/java/Duke.java
Outdated
|
||
String userInput = sc.nextLine().trim(); | ||
|
||
if (userInput.equals("bye")) { |
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.
Could modify so that it is not case-sensitive
src/main/java/Duke.java
Outdated
runLoop = false; | ||
} else if (userInput.equals("list")) { | ||
System.out.println("\t------------------------------------------"); | ||
if (tasks.size() == 0) { |
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 to check if the list is empty before printing out the list!
src/main/java/Duke.java
Outdated
} else { | ||
// adds a task to the list | ||
System.out.println("\t------------------------------------------"); | ||
switch(userInput.split(" ")[0]) { |
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.
could make a method to add task make code neater!
src/main/java/Duke.java
Outdated
break; | ||
case "event": | ||
// adds event tasks | ||
Task event = new Event(userInput.substring(6, userInput.indexOf("/at")), |
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.
userInput.indexOf("/at")+4 could be confusing
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 work! I think you follow code naming conventions and coding standards well. I think you could work on simplifying and breaking down complicated expressions and methods; readability is the name of the game here. Additionally, consider code organisation; you could split your code into more classes, as a lot of your program's functionality seems to be happening in just one class.
|
||
public Deadline(String description, String by) { | ||
super(description); | ||
this.by = by; |
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.
Consider using a more descriptive variable name that better illustrates what the variable represents.
} | ||
|
||
public String getStatusIcon() { | ||
return (isDone ? "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.
Though I understand what you're doing here, from a code readability standpoint, it's best to avoid conditionals in favour of a more explicit and clear if-else statement.
|
||
public static void exitProgram() { | ||
// exits the program | ||
System.out.println("\t------------------------------------------"); |
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.
If you're using this string a lot, perhaps consider storing it as a constant variable that you could re-use elegantly.
} | ||
|
||
public static void markTask(String userInput, ArrayList<Task> tasks) throws NumberFormatException, | ||
IndexOutOfBoundsException { |
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 that you followed wrapped-line indentation of 8 spaces!
src/main/java/Duke.java
Outdated
} else if (userInput.split(" ")[0].equalsIgnoreCase("done")) { | ||
markTask(userInput, tasks); | ||
} | ||
else if (userInput.split(" ")[0].equalsIgnoreCase("delete")) { |
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 could avoid complicated (and repeated) expressions by breaking it down into multiple lines and storing it as a variable first; I noticed the same issue in a few other classes too, and it would help a reader understand your code better!
System.out.println("\t__________________________________________\n"); | ||
} | ||
|
||
public static void addTask(String userInput, ArrayList<Task> tasks) throws StringIndexOutOfBoundsException { |
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.
This method is really long; consider breaking it down into smaller methods that would also make it easier for you to debug your own work. Maybe, upon doing so, you could move some of those methods to their relevant class (i.e. Deadline class, Event class, etc) to enhance code organisation
No description provided.