[Jerryl Chong] iP #127
[Jerryl Chong] iP #127
Conversation
This reverts commit 386e867.
There was a problem hiding this comment.
LGTM. Overall, good job on the OOP-aspects, as I can see the different kinds of types used. (Enum, abstract classes). It is meticulous of you to add specific Duke exception on top of it, and separating many components into classes. Other than a few missing JavaDocs, looks good to me. 👍
| @@ -0,0 +1,222 @@ | |||
| package duke; | |||
| import static java.lang.Integer.parseInt; | |||
There was a problem hiding this comment.
Maybe it would be nicer if you leave a line after package for your static import.
| * Finds tasks matching the given keyword. | ||
| * | ||
| * @param keyword | ||
| * @return |
There was a problem hiding this comment.
I think you forgot to fill in "@return ..."?
| import duke.Ui; | ||
| import duke.exception.DukeException; | ||
|
|
||
| public class FindCommand extends Command { |
There was a problem hiding this comment.
The public class and its methods are missing the JavaDoc.
| @@ -0,0 +1,11 @@ | |||
| package duke.exception; | |||
|
|
|||
| public class DukeException extends Exception { | |||
There was a problem hiding this comment.
The custom DukeException should have JavaDoc too, to describe briefly what it does?
| * @param s The description of the Deadline task. | ||
| * @param dateTime The due date of the Deadline task. | ||
| */ | ||
| public Deadline(String s, LocalDateTime dateTime) { |
There was a problem hiding this comment.
The param "s" could perhaps have a more meaningful name, like "description"?
| * @param s The description of the Event task. | ||
| * @param dateTime The date of the Event task. | ||
| */ | ||
| public Event(String s, LocalDateTime dateTime) { |
|
|
||
| @Test | ||
| public void parse_doneWithNonIntegerValue_exceptionThrown() { | ||
|
|
There was a problem hiding this comment.
I think this is still a work-in-progress right?
|
|
||
| import duke.task.Task; | ||
|
|
||
| public class ToDoStub extends Task { |
There was a problem hiding this comment.
You might need a JavaDoc here as it is a public class.
There was a problem hiding this comment.
Looks good! 👍
Apart from minor nits in the naming of some variables and methods, your code is clean and readable. I liked that you have very granular Exception checking. You might want to consider writing tests for your Storage class since it is a core class to your application.
| * | ||
| * @param s The given String | ||
| */ | ||
| public void say(String s) { |
There was a problem hiding this comment.
Would print be a better way to convey the behavior of the say method?
| import duke.exception.DukeException; | ||
| import duke.task.Task; | ||
|
|
||
|
|
There was a problem hiding this comment.
Would it be better to remove the additional line-breaks in this section?
|
|
||
| @SuppressWarnings("ALL") | ||
| public class TaskList { | ||
| private ArrayList<Task> list; |
There was a problem hiding this comment.
Would naming the list variable as tasks better convey the contents of the ArrayList?
| * @return The description of the marked task. | ||
| * @throws DukeException If there is no task with the given index. | ||
| */ | ||
| public String done(int index) throws DukeException { |
There was a problem hiding this comment.
done might be a little ambiguous. Would a better name be markAsDone?
| * | ||
| * @return The task in a Storage readable format. | ||
| */ | ||
| public abstract String toCommand(); |
There was a problem hiding this comment.
Would toReadableTaskFromat be a more accurate description of this method?
| import duke.exception.DukeException; | ||
|
|
||
| public class DueInCommand extends Command { | ||
| private long time; |
There was a problem hiding this comment.
I am not sure what this variable is for. Similarly for hour. Would it be better to rename this variable to be more descriptive?
A-Assertion branch
A-CodeQuality branch
No description provided.