-
Notifications
You must be signed in to change notification settings - Fork 77
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
[Deen Liong] iP #83
base: master
Are you sure you want to change the base?
[Deen Liong] iP #83
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.
Hi Deen, good job on the iP so far! I have added some comments on the code quality, coding standard violations and naming conventions. Hope they will be helpful. 👍🏽
System.out.println("What can I do for you?"); | ||
} | ||
|
||
public static void main(String[] args) { |
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 function is very long (>30lines). Do consider breaking it down into multiple smaller functions.
src/main/java/duke/TaskManager.java
Outdated
|
||
public class TaskManager { | ||
|
||
public static void printSuccessfulAdd(Task[] tasks, int taskCount) { |
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 avoid the usage of magic literals and consider refactoring them to final
variables instead.
src/main/java/duke/TaskManager.java
Outdated
|
||
public static void printTaskList(Task[] tasks, int taskCount) { | ||
System.out.println("\t_____________________"); | ||
// if there are no 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.
This comment may be redundant. Consider removing it
package duke.exception; | ||
|
||
public abstract class DukeException extends Exception { | ||
//no other code needed |
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 don't need to add comments where it is perhaps not necessary to do so.
src/main/java/duke/task/Task.java
Outdated
} | ||
|
||
public String getStatusIcon() { | ||
if (isDone) { // 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.
Maybe remove the comment here since it is redundant and the code explains itself well already.
src/main/java/duke/task/Task.java
Outdated
public String toString() { | ||
|
||
return getStatusIcon() + description; | ||
// return getStatusIcon() ; |
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.
Avoid leaving commented out code in your codebase, since it reduces readability
small bug - bye now prints error message
1 - bye error message still show 2 - printing of number of items in inputFile at the start
Branch level 9
Branch level 9
Added User Guide
No description provided.