-
Notifications
You must be signed in to change notification settings - Fork 438
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
[Ek Chin Hui] iP #452
base: master
Are you sure you want to change the base?
[Ek Chin Hui] iP #452
Conversation
# Conflicts: # src/main/java/Duke.java
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.
Nice code, LGTM!
src/main/java/duke/Task.java
Outdated
* Getter for isDone. | ||
* @return boolean of the status of the task. | ||
*/ | ||
public boolean getIsDone() { |
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.
Not sure whether you really need a getter when you can make the isDone boolean public
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 quality code, just a few nits to fix.
src/main/java/duke/Deadline.java
Outdated
|
||
/** | ||
* Constructor for Deadline, initialises description | ||
* in the base class and also the dueDate. |
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 an empty line between description and parameter section of javadoc?
|
||
/** | ||
* Constructor for Duke class, initialises objects | ||
* for Storage, TaskList and Ui. |
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 an empty line between description and parameter section of javadoc? I noticed the same issue in several other places too.
/** | ||
* Base class for custom exceptions specific to Duke. | ||
*/ | ||
public class DukeException extends Exception{ |
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 spacing after "Exception" and before the curly brace?
src/main/java/duke/Deadline.java
Outdated
String month = dueDate.getMonth().getDisplayName(TextStyle.SHORT, Locale.forLanguageTag("en")); | ||
int day = dueDate.getDayOfMonth(); | ||
int year = dueDate.getYear(); | ||
String timeDisplay = String.format("%d %s %d", day, month, year); | ||
return "[D]" + super.toString() + String.format(" (by: %s)", timeDisplay); |
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 it might be good to separate logical units in a block of code with blank lines?
src/main/java/duke/Event.java
Outdated
String month = time.getMonth().getDisplayName(TextStyle.SHORT, Locale.forLanguageTag("en")); | ||
int day = time.getDayOfMonth(); | ||
int year = time.getYear(); | ||
String timeDisplay = String.format("%d %s %d", day, month, year); | ||
return "[E]" + super.toString() + String.format(" (at: %s)", timeDisplay); |
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 think it might be good to separate logical units in a block of code with blank lines. I noticed the same issue in several other places too.
src/main/java/duke/Parser.java
Outdated
ui.printMessage("Added: " + task | ||
+ String.format("\nNow you have %d tasks in the list", taskList.getTaskList().size())); |
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 break before an operator and after a comma, I have also noticed this in other sections of your code! 👍
Add assert statements
Refactor storage class
No description provided.