Conversation
danielonges
left a comment
danielonges
left a comment
There was a problem hiding this comment.
Overall, it was a pleasant experience reading your code! No major coding standard violations were found, just some minor tweaks that might need to be done here and there. Otherwise, LGTM!
src/main/justin/Justin.java
Outdated
| ui.showHelpMessage(); | ||
|
|
||
| // Condition for Duke to stop | ||
| boolean terminate = false; |
There was a problem hiding this comment.
Should this be canTerminate? Might be better to change to reflect boolean variable name
src/main/justin/Storage.java
Outdated
| } | ||
| } else if (task.getList().get(i) instanceof Deadline) { // is a deadline class | ||
| if (task.getList().get(i).isDone) { | ||
| holder = "D" + "|" + "1" + "|" + task.getList().get(i).description + "|" + ((Deadline) task.getList().get(i)).by; |
src/main/justin/Ui.java
Outdated
| printSpace(); | ||
| System.out.println("To add a todo: use command todo<space>taskName"); | ||
| System.out.println("To add a deadline: use command deadline<space>taskName<space>/by<space>YYYY-MM-DD"); | ||
| System.out.println("To add a event: use command event<space>taskName<space>/at<space>YYYY-MM-DD<space>HH:MM"); |
src/main/justin/Ui.java
Outdated
|
|
||
| static String logo = | ||
|
|
||
| " ,--. ,--. ,--. \n" + |
There was a problem hiding this comment.
Might want to consider shifting "+" operator to begin on new line
|
|
||
| @Override | ||
| public String toString() { | ||
| return "[E]" + super.toString() + " (at: " + date1.format(DateTimeFormatter.ofPattern("MMM d yyyy")) + " " + |
Update JUnit testing
weixue123
left a comment
weixue123
left a comment
There was a problem hiding this comment.
Hi Wei Kiat, I feel that your code is quite well segmented. Great work!
src/main/java/justin/Justin.java
Outdated
|
|
||
| case "DONE": | ||
|
|
||
| String num = text.substring(5); // take out the int value of the task to be completed |
There was a problem hiding this comment.
Would it be clearer if a named constant or method was used to extract the task number instead? (instead of using a "magic" number)
src/main/java/justin/Justin.java
Outdated
| ui.showHelpMessage(); | ||
|
|
||
| // Condition for Duke to stop | ||
| boolean terminate = false; |
There was a problem hiding this comment.
I feel that the code is clear enough for certain comments to be omitted!
src/main/java/justin/Storage.java
Outdated
| } | ||
| } else if (task.getList().get(i) instanceof Deadline) { // is a deadline class | ||
| if (task.getList().get(i).isDone) { | ||
| holder = "D" + "|" + "1" + "|" + task.getList().get(i).description + "|" + ((Deadline) task.getList().get(i)).by; |
There was a problem hiding this comment.
Perhaps use a method to reduce the code duplication here?
| if (text.length() < 5 && text.contains("todo") ) { // case 1 | ||
| throw new JustinException("☹ OOPS!!! The description of a todo cannot be empty."); | ||
| } | ||
| else if (text.contains("blah")) { // case 2 |
There was a problem hiding this comment.
According to convention and for better readability, maybe consider indenting the trailing comments to the same tab setting?
|
|
||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
I think there are times when it is not really necessary to have multiple blank lines in between parts of your code?
PR from branch-A-CodeQuality to master
PR from branch-A-JavaDoc to master
PR branch-A-Assertions
PR branch-Level-10
PR branch-A-CodingStandard
PR branch-Level-9
3 participants
Reopening my current PR based on feedbacks from Prof