[Koh Han Ming] iP #401
[Koh Han Ming] iP #401
Conversation
src/main/java/Parser.java
Outdated
| } else if (input.equals("find")) { | ||
| if (taskList.isEmpty()) { | ||
| throw new EmptyListException("There are no tasks on your list"); | ||
| } |
There was a problem hiding this comment.
Perhaps it might be better to check if taskList.isEmpty() at the start so as to avoid deep nesting? Or maybe it would be good to create a method that checks if the taskList is empty so as to avoid code duplication.
There was a problem hiding this comment.
Also, perhaps it would be great to split up this long method to shorter ones?
There was a problem hiding this comment.
I'd concur with the views above - hope the changes aren't much trouble :))
There was a problem hiding this comment.
Oh that makes a lot of sense. It prevents duplicate code too. Thanks a lot guys! :)
src/main/java/Storage.java
Outdated
| @@ -0,0 +1,81 @@ | |||
| import java.io.*; | |||
There was a problem hiding this comment.
Maybe it might be better to explicitly list the files you are importing instead of importing all the files?
There was a problem hiding this comment.
Explicitly listed, the classes would be BufferedReader, BufferedWriter, File, FileReader and FileWriter.
src/main/java/Todo.java
Outdated
| * Creates a Todo object | ||
| * @param description Description of task. | ||
| * @param taskType Type of task. | ||
| */ |
There was a problem hiding this comment.
perhaps it would be good to have an empty line between the descriptions and parameters.
There was a problem hiding this comment.
LGTM. The code style and quality were almost impeccable - I loved how the unit tests were written! Nit: Maybe you could start commit messages with an imperative verb to comply with the SE-EDU guidelines.
src/main/java/Deadline.java
Outdated
| <<<<<<< HEAD | ||
| /** | ||
| * Supports the creation of Deadline objects. | ||
| */ | ||
| public class Deadline extends Task{ | ||
| ======= | ||
| public class Deadline extends Task { | ||
| >>>>>>> branch-A-CodingStandard |
There was a problem hiding this comment.
Perhaps you missed out a merge conflict here?
There was a problem hiding this comment.
I did, thanks for pointing it out!
src/main/java/Parser.java
Outdated
| } else if (input.equals("find")) { | ||
| if (taskList.isEmpty()) { | ||
| throw new EmptyListException("There are no tasks on your list"); | ||
| } |
There was a problem hiding this comment.
I'd concur with the views above - hope the changes aren't much trouble :))
src/main/java/Storage.java
Outdated
| @@ -0,0 +1,81 @@ | |||
| import java.io.*; | |||
There was a problem hiding this comment.
Explicitly listed, the classes would be BufferedReader, BufferedWriter, File, FileReader and FileWriter.
Add Assertions
Add CodeQuality
Add extension
No description provided.