-
Notifications
You must be signed in to change notification settings - Fork 364
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
[minosx31] iP #353
base: master
Are you sure you want to change the base?
[minosx31] iP #353
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.
Overall okay so far, just a few key points to take note of to change. Some of the changes such as simplifying the long if-else/switch case would come naturally after the next few levels.
Keep up the good work! :D
src/main/java/Duke.java
Outdated
private static String numOfTasks(ArrayList<Task> tasks) { | ||
return tasks.size() == 1 ? " 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.
The method name could perhaps be changed as it is misleading. numOfTasks would normally imply returning the number of tasks the list contain. However, you are just getting the string of whether it task should be plural or not. Method names should also start with a verb instead.
src/main/java/Duke.java
Outdated
String logo = " ____ _ \n" | ||
+ "| _ \\ _ _| | _____ \n" | ||
+ "| | | | | | | |/ / _ \\\n" | ||
+ "| |_| | |_| | < __/\n" | ||
+ "|____/ \\__,_|_|\\_\\___|\n"; | ||
System.out.println("Hello from\n" + logo); | ||
|
||
System.out.println("Hello I'm Duke!\n" + "How can I help you?"); |
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 you can take out the string in your println to avoid "magic" strings. Such as welcomeMsg = "...". There are several instances later on as well that you can take not of.
src/main/java/Deadline.java
Outdated
public String toString() { | ||
return "[D]" + "[" + super.getStatusIcon() + "] " + super.getDescription() + " (by: " + this.doBy + ")"; | ||
} |
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 feel like there is no need to use super.getStatusIcon
and super.getDescription
since your Deadline already extends from Tasks. Hence, these methods already exist within the Deadline class. Furthermore, the use of this should be removed if there is no multiple instances of it. This is a suggestion.
public String toString() { | |
return "[D]" + "[" + super.getStatusIcon() + "] " + super.getDescription() + " (by: " + this.doBy + ")"; | |
} | |
public String toString() { | |
return String.format("[D] [%s] %s (by: %s)", getStatusIcon(), getDescription(), doBy); | |
} |
The other classes that extend from Task have similar problem and can be dealt with respectively as well.
src/main/java/Duke.java
Outdated
if (remainingInput == null) { | ||
throw new IncorrectInputException("Enter a 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.
I like that you did this so that it is easier to see the "happy path" that the code would traverse through.
src/main/java/Duke.java
Outdated
if (tasks.size() != 0) { | ||
for (int i = 0; i < tasks.size(); i++) { | ||
System.out.println((i + 1) + "." + tasks.get(i).toString()); | ||
} | ||
} |
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 the if condition can be taken out so that it is easier to see the "happy path" and reduce indentation of code.
if (tasks.size() != 0) { | |
for (int i = 0; i < tasks.size(); i++) { | |
System.out.println((i + 1) + "." + tasks.get(i).toString()); | |
} | |
} | |
if (tasks.size() == 0) { | |
continue; | |
} | |
for (int i = 0; i < tasks.size(); i++) { | |
System.out.println((i + 1) + "." + tasks.get(i).toString()); | |
} |
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! I have commented on a few suggestions but generally, I think your code is easy to read! I like how you extracted the Task class very neatly into the 3 other classes.
src/main/java/Duke.java
Outdated
} | ||
} | ||
|
||
private static String numOfTasks(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.
I agree with @LiuMC-SG, perhaps this can be a suggestion?
private static String numOfTasks(ArrayList<Task> tasks) { | |
private static String getTaskString(ArrayList<Task> tasks) { |
src/main/java/Duke.java
Outdated
public static void main(String[] args) { | ||
ArrayList<Task> tasks = new ArrayList<>(); | ||
Scanner sc = new Scanner(System.in ); |
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 there be no extra space after System.in
?
Scanner sc = new Scanner(System.in ); | |
Scanner sc = new Scanner(System.in); |
src/main/java/Event.java
Outdated
|
||
@Override | ||
public String toString() { | ||
return "[E]" + "[" + super.getStatusIcon() + "] " + super.getDescription() + " (from: " + this.period[0] + " to: " + this.period[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.
Would it be better to use line break as the character count exceeds 120?
src/main/java/Duke.java
Outdated
if (remainingInput == null) { | ||
throw new IncorrectInputException("Enter a description with a deadline!"); | ||
} | ||
String[] deadlineDetails = remainingInput.split(" /by ", 2); | ||
String deadlineDescription = deadlineDetails[0]; | ||
String deadline = deadlineDetails.length > 0 ? deadlineDetails[1] : null; | ||
|
||
if (deadline == null) { | ||
throw new IncorrectInputException("Enter a valid deadline!"); | ||
} | ||
Task newTask = new Deadline(deadlineDescription, deadline); | ||
tasks.add(newTask); | ||
System.out.println("Got it. I've added this task:"); | ||
System.out.println(newTask); | ||
System.out.println("Now you have " + tasks.size() + Duke.numOfTasks(tasks) + "in the 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.
I like how the different logical units are separated by a line! The same goes for the rest of this if statement.
The current Command classes that are responsible for handling commands that involve creating a new task, assumes that a Task object will be created upon execution. These classes are: * DeadlineCommand * EventCommand * TodoCommand These assumptions will have to hold in order for these classes to work as intended. However, if they do not hold, it will result in errors in other areas of the code. Having assertions will ensure that the original assumption holds in the respective Command classes.
Ensure that the code follows the provided guidelines so that it remains readable and is of high quality.
Add assertions
Improve code quality
MarkCommand does not handle the scenario where a task number does not exist in the TaskList. Todo, Event and Deadline also does not handle scenarios where task details are empty or invalid. When a user enters an input that belongs to such scenarios, they may encounter runtime errors which are undesirable. The respective classes can now handle errorneous inputs such as: * marking/unmarking a task number that does not exist * wrong command format * empty task descriptons
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:
If you are a Java programmer, you can use it to practice Java too. Here's the
main
method: