[Jeremykhoo] iP #360
[Jeremykhoo] iP #360
Conversation
src/main/java/Deadline.java
Outdated
| protected String by; | ||
| public Deadline(String description, String by) { | ||
| super(description); | ||
| this.by = by; | ||
| } | ||
|
|
||
| @Override | ||
| public String toString() { | ||
| return "[D]" + super.toString() + " (by: " + by + ")"; |
There was a problem hiding this comment.
I believe this file has extra level of indentations here weirdly. I notice the same issue occurring in other files too...
src/main/java/Duke.java
Outdated
|
|
||
| private static void addList(Task task) { | ||
| list.add(task); | ||
| System.out.println("added: " + task.getDescription()); |
There was a problem hiding this comment.
Could it better if we just use the task's string representation here instead? This would consequently remove the need for an extra getter in the Task's class
src/main/java/Duke.java
Outdated
| private static void deleteTask(int i) { | ||
| int index = i - 1; | ||
| System.out.println("removed: " + list.get(index).toString()); | ||
| System.out.print("You have: " + (list.size() - 1)+ " task(s)"); |
There was a problem hiding this comment.
Missing trailing whitespace before '+'. This is a common issue found in other part of the codebase too.
src/main/java/Duke.java
Outdated
| catch (IllegalArgumentException e) { | ||
| throw new DukeException(e); | ||
| } | ||
|
|
||
| catch (Exception e) { | ||
| throw new DukeException(e); | ||
| } |
There was a problem hiding this comment.
How about shifting your try-catch logic wrapped around the switch-case block instead? That would reduce some code duplication when the same exception could be thrown
src/main/java/Duke.java
Outdated
| parseIn(parm); | ||
| } | ||
| catch (DukeException e) { | ||
|
|
There was a problem hiding this comment.
I see that the DukeException will send a message to user when it is created, I believe adding a comment for this behaviour would be helpful for others to understand why there is an empty catch block.
src/main/java/Duke.java
Outdated
| int byIndex; | ||
| int fromIndex; | ||
| String description; | ||
| List<String> l; |
There was a problem hiding this comment.
List<String> l could be given a more descriptive variable name, similar for variable names like f and t
src/main/java/Duke.java
Outdated
| public class Duke { | ||
| public static void main(String[] args) { | ||
| private static ArrayList<Task> list; |
There was a problem hiding this comment.
This is an interesting concept of ensuring only one instance of task list exists. Any reason it is designed this way? If new Duke was somehow initialised, the array list is 'resetted' base on how the constructor is written now
src/main/java/Task.java
Outdated
| this.isDone = false; | ||
| } | ||
|
|
||
| public String getStatusIcon() { |
There was a problem hiding this comment.
This could be set to private since it is only used internally within the class
src/main/java/Deadline.java
Outdated
| @@ -0,0 +1,13 @@ | |||
| public class Deadline extends Task { | |||
|
|
|||
| protected String by; | |||
There was a problem hiding this comment.
There seems to be extra indentation for every line in not just this file, but the other files as well.
src/main/java/Duke.java
Outdated
| } | ||
| break; | ||
| case "event": | ||
| try{ |
There was a problem hiding this comment.
Maybe leave a space between the try and starting curly bracket?
Branch a assertions
Branch a coding quality
DukeExtended
duke now can:
remember to do things - dukehelp you be productiveall you need is :
2. patients to learn
there are even task lists:
main is