[Toh Zhen Yu, Nicholas] iP #457
Conversation
had to compile in java 8 due to runtime environment on local
fix indexing bug abstract number of tasks message delete command
using java serialization needs work handling non existant directories
not necessary for tasks to have a storage object
add private constructor remove unused store method
Empty commit: project already follows a coding standard
Add javadoc for find Refactor print_tasks to use UI
| String[] subst; | ||
| LocalDate date; | ||
| switch (command) { | ||
| case "list": |
There was a problem hiding this comment.
This is incorrect because there should not be any indentation for the case clauses.
src/main/java/Deadline.java
Outdated
| public class Deadline extends Task { | ||
|
|
||
| /** | ||
| * @param description Task description |
There was a problem hiding this comment.
There should be a punctuation behind every parameter description. I have noticed similar issue in other places too.
src/test/java/TaskTest.java
Outdated
| @Test | ||
| public void dummyTest(){ | ||
| Task t = new Task("test1", LocalDate.parse("2001-09-11")); | ||
| // System.out.println(t.toString()); |
There was a problem hiding this comment.
This is incorrect as comments should be indented relative to their position in the code.
| @@ -1,10 +1,34 @@ | |||
| /** | |||
There was a problem hiding this comment.
Perhaps putting the class in a package?
src/main/java/Storage.java
Outdated
| @@ -0,0 +1,55 @@ | |||
| import java.io.*; | |||
There was a problem hiding this comment.
The imported classes should be listed explicitly.
src/main/java/Task.java
Outdated
| */ | ||
| public class Task implements java.io.Serializable { | ||
| String text; | ||
| boolean done; |
There was a problem hiding this comment.
Perhaps a more intuitive variable name that will sound more like a boolean?
|
|
||
| /** | ||
| * Entry point of the duke program. Read from storage and creates a UI class instance. | ||
| * Serves as intermediate between the UI and parser. |
There was a problem hiding this comment.
This is incorrect as there should be an empty line between the description and parameter. I have noticed similar issues in several other places too.
src/main/java/Deadline.java
Outdated
| */ | ||
| public class Deadline extends Task { | ||
|
|
||
| /** |
There was a problem hiding this comment.
Perhaps adding a short summary of the method? This can also apply to other methods that do not have a description in other places.
There was a problem hiding this comment.
thanks, not sure how I should describe the constructor or toString, do you have suggestions?
There was a problem hiding this comment.
All in all, the code was easy to read and understand. There was no deep nesting done, and the code was not complicated. The names of the variables made sense in accordance to the methods. However, some form of packaging can be done to make the organisation of the code cleaner!
There was a problem hiding this comment.
Your code is quite neat and easy to follow. There is no overly complicated code that needs abstraction. I agree with most of the changes suggested by Door-oof and suggested some minor violations that he might have missed out.
Besides packaging, I also recommend considering the usage of public vs protected access modifiers as I noticed that public is used for certain methods while protected is used for others, even though both methods should have the same accessibility level.
Overall, it's quite a good piece of work and a pleasure to read.
| case "done": | ||
| case "delete": |
There was a problem hiding this comment.
The "done" case should end with a break; statement. You might have unintentionally shifted the "done" case into the "delete" case.
There was a problem hiding this comment.
Hi, this was intentional as both commands use the same logic of tokenizing the input to get the task number, then getting that task object. It is after these steps where the logic then differs. Do you have a suggestion of a better way to do this?
src/main/java/Parser.java
Outdated
| break; | ||
| default: | ||
| throw new DukeException("I'm sorry, but I don't know what that means :-("); | ||
| // break; |
There was a problem hiding this comment.
A break statement should be included for the default case too.
There was a problem hiding this comment.
Hmm intellij complains about this as it's an unreachable statement. Do you know how I can configure it to ignore this error, or have a suggestion on how to implement this logic instead?
# Conflicts: # .gitignore # build.gradle
improve code quality
add assertions
No description provided.