[Yoon Jia Jun Ken] Duke Increments#364
[Yoon Jia Jun Ken] Duke Increments#364Xelyion wants to merge 50 commits intonus-cs2103-AY1920S1:masterfrom
Conversation
Add toolVersion block in to Gradle code sample to prevent errors.
Change file mode on `gradle` to be executable (nus-cs2103-AY1920S1#9)
Gradle defaults to an empty stdin which results in runtime exceptions when attempting to read from `System.in`. Let's add some sensible defaults for students who may still need to work with the standard input stream.
Add configuration for console applications
# Conflicts: # src/main/java/Task.java
# Conflicts: # src/main/java/Deadline.java # src/main/java/Event.java
|
Hello Xelyion! I understand that the pull request should be done with your project's master branch, but as the bulk of your business logic is in your previous pull request (branch-Level-9), I have made my comments there instead. |
ZhangHuafan
left a comment
ZhangHuafan
left a comment
There was a problem hiding this comment.
Hi, Jia Jun! I think your JavaDoc is very detailed and most of them are following the correct formats. The logic of indvidual parts is mostly clear as well. However, you may want to refactor some parts so that the whole structure could be more reasonable.
| * Adds Deadlines: "deadline". | ||
| * Adds Events: "event". | ||
| * Adds ToDos: "todo". | ||
| * |
There was a problem hiding this comment.
Very detailed Javadoc but may consider writing more clear explanations.
|
|
||
| public String execute(Ui ui, TaskList taskList, Storage storage) throws DukeException { | ||
| return ""; | ||
| } |
There was a problem hiding this comment.
May consider making this method as abstract so that you do not need to return a default string.
| * An abstract class command, created from the Parser. | ||
| * Contains Strings to store the command and type of command. | ||
| */ | ||
|
|
There was a problem hiding this comment.
I think there is no line break between them =)
| } | ||
|
|
||
|
|
||
| } |
There was a problem hiding this comment.
Personally, I think the child classes of Command can self-explain their "type". Maybe you could consider making full use of this.
| } else { | ||
| throw new DukeException(" Date is not in the format: (DD/MM/YYYY HHMM)"); | ||
| } | ||
| } |
There was a problem hiding this comment.
For parsing date string to a date object. You may find LocalDateTime.parse() method useful. You may want to refer to this link: https://www.baeldung.com/java-string-to-date. Besides, would it be better if this method belongs to Parse class?
| return command; | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
It is very nice that you added "save" command! However, you may consider using switch and case to make the code structure clearer. Besides, you could consider using the String.split().
5 participants
No description provided.