-
Notifications
You must be signed in to change notification settings - Fork 270
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
[Gernene Tan] iP #290
base: master
Are you sure you want to change the base?
[Gernene Tan] iP #290
Conversation
IP requirement
Add asserts
Refactor Parser.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.
Overall, I found your code easy to read and they follow the Java coding standard provided. However, some files such as Parser.java were difficult to read due to the nested if-else statements. Consider encapsulating some similar statements into a function to improve readability.
src/main/java/duke/task/Task.java
Outdated
return isMarked; | ||
} | ||
|
||
public boolean contains(String str) { |
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 change to the function name to sound more like a boolean?
public boolean contains(String str) { | |
public boolean hasString(String str) { |
src/main/java/duke/task/Todo.java
Outdated
|
||
public class Todo extends Task { | ||
|
||
public Todo(String description, boolean status) { |
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 change to the argument name to sound more like a boolean? I noticed this issue occurring in classes extending task.
public Todo(String description, boolean status) { | |
public Todo(String description, boolean isMarked) { |
src/main/java/duke/task/Event.java
Outdated
public class Event extends Task { | ||
|
||
private LocalDate date; | ||
|
||
public Event(String description, boolean status, LocalDate date) { | ||
super(description, status); | ||
this.date = date; | ||
} | ||
|
||
public Event(String description, LocalDate date) { | ||
this(description, false, date); | ||
} | ||
|
||
public String toString() { | ||
return String.format("[E][%s] %s (at: %s)", super.isMarked(), super.getDescription(), DateTimeFormatter.ofPattern("MMM d yyyy")); | ||
} | ||
} |
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 add header comments for the class and public functions. I noticed the same issue for classes extending Task and Command?
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 your code closely follows the coding standard and is nicely constructed. Only some small writing problems to fix.
this.task = task; | ||
} | ||
|
||
public void execute(TaskList tasks, Ui ui, Storage storage) { |
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.
Although it is not required, it is recommended to add the '@ Override' annotation. https://stackoverflow.com/questions/1005898/should-i-add-an-override-annotation-when-implementing-abstract-methods-in-java
|
||
public class DukeException extends Exception { | ||
|
||
public DukeException(String message) { |
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.
According to the stupid Coding standard, should it be better to indent this line as 4 spaces instead of 2?
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.
Excellent attempt at following coding standard with absolutely minimal coding standard violations
src/main/java/duke/task/Event.java
Outdated
|
||
private LocalDate date; | ||
|
||
public Event(String description, boolean status, LocalDate date) { |
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 name status to be more boolean-sounding? :)
src/main/java/duke/Parser.java
Outdated
* @return UnmarkCommand instance. | ||
*/ | ||
public Command parseUnmarkCommand(String fullCommand) { | ||
String dataString = fullCommand.replaceFirst("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.
Commendable discretion shown here to differentiate when to use camelCase and when to use PascalCase.
src/main/java/duke/Storage.java
Outdated
assert data[1].equals("true") || data[1].equals("false") : throw DukeException("Invalid file input"); | ||
Boolean status = Boolean.parseBoolean(data[1]); | ||
String text = data[2]; | ||
if (data[0].equals("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.
The if-else class of statements should have the following form:
if (condition) {
statements;
} else if (condition) {
statements;
} else {
statements;
}
} | ||
|
||
public String toString() { | ||
return String.format("[D][%s] %s (by: %s)", super.isMarked(), super.getDescription(), DateTimeFormatter.ofPattern("MMM d yyyy")); |
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 line is rather long and would benefit from a line break?
Branch wk5
Unit tests for Parser and Storage are incomplete. Adding more test methods helps ensure code quality. Let's add unit tests methods to ensure storage and command line data is interpreted correctly.
Prepare duke for release
Fix dialog text wrapping
Duke
Create
Deadlines
Events
Todo Items
Check off items
Mark tasks as not done
list
😄
Download here