[Cao Peng] iP #236
[Cao Peng] iP #236
Conversation
| private Task parseTask(String taskType, String taskStr) throws DukeException { | ||
| String pattern; | ||
| Pattern r; | ||
| Matcher m; |
There was a problem hiding this comment.
A more meaningful naming suggestion: String input, Pattern pattern, Matcher matcher. I found this issue in the Storage class as well.
| * @return The task list which is loaded from the file. | ||
| * @throws DukeException Exception if there is error when loading from the file. | ||
| */ | ||
| public ArrayList<Task> load() throws DukeException { |
There was a problem hiding this comment.
Can this method name be loadTasksFromStorage to explain the method more intuitively?
| * @return A duke.task.Task object which represents a task as the string. | ||
| * @throws DukeException Exception if there is error when parsing the task string. | ||
| */ | ||
| public Task parseTask(String str) throws DukeException { |
There was a problem hiding this comment.
Should this method be private instead, because it is independent of the whole program and is only used by the load method from the same class as an encapsulation?
| @@ -0,0 +1,101 @@ | |||
| package duke; | |||
|
|
|||
| import duke.command.*; | |||
There was a problem hiding this comment.
Java coding standards stated that the imported classes should always be listed explicitly. I understand why you have imported * from duke.command package, but to comply with the guideline, would it be better to import explicitly instead?
| /** | ||
| * Deletes a task specified by task id from the task list. | ||
| * @param taskId The task id of the task which will be deleted. | ||
| * @return The task which is deleted. | ||
| * @throws DukeException Exception if there is error when deleting the task. | ||
| */ |
There was a problem hiding this comment.
Do note that there should be an empty line between the description and the parameters. I noticed the same issue in your other JavaDoc comments as well. Perhaps consider configuring your IDE to help with this styling?
| public class DeadlineTest { | ||
| @Test | ||
| public void deadlineTest() { | ||
| assertEquals(new Deadline("return book", LocalDate.parse("2019-10-15")).toString(), "[D][ ] return book (by: Oct 15 2019)"); |
There was a problem hiding this comment.
This line has exceeded the limit of 120 words. Would it be better to separate it into 2 lines instead?
src/main/java/duke/Parser.java
Outdated
|
|
||
| try { | ||
| switch (parsedCommand[0]) { | ||
| case "done", "delete": |
There was a problem hiding this comment.
Do note that there should be no indentation for case clauses as to adhere to the Java coding standard. I noticed the same issue in the other classes as well. Perhaps consider configuring your IDE to help with this styling?
| import java.util.ArrayList; | ||
|
|
||
| public class FindCommand extends Command { | ||
| String keyword; |
There was a problem hiding this comment.
Despite not really violating the coding standards as it only mentioned that variable shouldn't be public, but since other classes or sub-classes are not accessing the keyword variable, would it be better to declare it as private?
| package duke; | ||
| import duke.exception.DukeException; |
There was a problem hiding this comment.
Not explicitly stated in the Java coding standard but would it be neater and consistent if there is a breakline between package and import?
| /** | ||
| * @param fullCommand the user command | ||
| * @return a duke.command.Command object which encapsulates the information of a parsed user command | ||
| * @throws DukeException exception when there is an parsing error | ||
| */ |
There was a problem hiding this comment.
Maybe a short summary of the method could be written as the first sentence for this method header.
| } else if (fullCommand.equals("list")) { | ||
| return new ListCommand(); | ||
| } else { | ||
| String[] parsedCommand = fullCommand.split(" "); |
There was a problem hiding this comment.
Plural form could be used for this String array to make the variable name sound more like a collection of string values.
| public Event(String title, boolean isDone, LocalDate date) { | ||
| super(title, isDone); | ||
| this.date = date; | ||
| } | ||
|
|
||
| public Event(String title, LocalDate time) { | ||
| this(title, false, time); | ||
| } |
There was a problem hiding this comment.
Would it be better if javadoc comments were included as there is more than one constructor?
Add an assertion
Change switch case statement format
first pull request