[enamourous] iP#655
Conversation
wangyuanchi
left a comment
There was a problem hiding this comment.
The code is generally clean and good!
| switch (command) { | ||
| case "bye": |
There was a problem hiding this comment.
I think it would be better to align the switch and case statements to better match the coding guidelines.
| switch (command) { | |
| case "bye": | |
| switch (command) { | |
| case "bye": |
seokjoon27
left a comment
There was a problem hiding this comment.
Nicely done with coding conventions, all coding standards seems to be well adapted.
| @@ -0,0 +1,185 @@ | |||
| package ip; | |||
| /** | ||
| * The Note class represents a simple command-line task manager chatbot. | ||
| * Users can add todos, deadlines, and events, mark/unmark tasks as done, | ||
| * delete tasks, find tasks by keyword, and save/load tasks from a file. | ||
| */ |
There was a problem hiding this comment.
All methods and classes containing header comments
| * delete tasks, find tasks by keyword, and save/load tasks from a file. | ||
| */ | ||
| public class Note { | ||
| private static final String FILE_PATH = "data/duke.txt"; |
| */ | ||
| public class Task { | ||
| protected String description; | ||
| protected boolean isDone; |
ashmitahaldar
left a comment
There was a problem hiding this comment.
Overall great work! Apart from some small tweaks to adhere to coding standard, your chatbot aligns well with code quality standards with regards to naming 😄
| @@ -0,0 +1 @@ | |||
| T | 0 | eat rice | |||
There was a problem hiding this comment.
Perhaps you could add this data storage file to the .gitignore file, so that your saved task lists don't get added to the repository on GitHub!
| @@ -0,0 +1,50 @@ | |||
| package Note.ui; | |||
There was a problem hiding this comment.
Could task classes perhaps be placed in a separate package with a different name? Such as Note.task
| switch (command) { | ||
| case "bye": |
| for (Task t : tasks) { | ||
| String line = ""; | ||
| switch (t.getTypeIcon()) { // use getTypeIcon() instead of getType() | ||
| case "T": |
There was a problem hiding this comment.
It would be great if you could match the indentation of the switch and case staements here to align with the coding standard.
| case "T": | |
| switch (t.getTypeIcon()) { // use getTypeIcon() instead of getType() | |
| case "T": |
In build.gradle, the dependencies on distZip and/or distTar causes
the shadowJar task to generate a second JAR file for which the
mainClass.set("seedu.duke.Duke") does not take effect.
Hence, this additional JAR file cannot be run.
For this product, there is no need to generate a second JAR file
to begin with.
Let's remove this dependency from the build.gradle to prevent the
shadowJar task from generating the extra JAR file.
YSTey0808
left a comment
There was a problem hiding this comment.
Overall well done. Maybe can take note of packages and some code standard. Good job
| @@ -0,0 +1,47 @@ | |||
| package Note.ui; | |||
There was a problem hiding this comment.
I think it would be better to have more packages for your ui part. Maybe can abstract out tasks, then storage so on and so forth.
| @@ -0,0 +1,55 @@ | |||
| package Note.ui; | |||
| case "bye": | ||
| return "Bye! Hope to see you again soon!"; | ||
|
|
||
| case "list": |
There was a problem hiding this comment.
My idea is that would be better to abstract out those command implementation to a new method
| @@ -0,0 +1,111 @@ | |||
| package Note.ui; | |||
There was a problem hiding this comment.
Same idea. Abstract to new package would be better.
| try (BufferedWriter writer = new BufferedWriter(new FileWriter(filePath))) { | ||
| for (Task t : tasks) { | ||
| String line = ""; | ||
| switch (t.getTypeIcon()) { // use getTypeIcon() instead of getType() |
There was a problem hiding this comment.
Should take note of swtich case code style
| @@ -0,0 +1,36 @@ | |||
| package Note.ui; | |||
There was a problem hiding this comment.
Same idea. Abstract to new package would be better.
| @@ -0,0 +1,61 @@ | |||
| package Note.ui; | |||
The code for parsing dates and extracting command arguments relies on assumptions that are not explicitly documented in the code itself. For example, Deadline.parseDateTime assumes that the input string follows the d/M/yyyy HHmm format, while methods in Parser assume the presence of certain flags or spacing in user input. Without explicit documentation, such assumptions are easy to overlook. This can make debugging harder when unexpected input causes failures in non-obvious places. Let's add Java assert statements in critical places to document these assumptions. Assertions make the expectations clear to both the developer and future maintainers, while also allowing failures to be detected early during testing. Using assert instead of exceptions is appropriate here because: These conditions reflect developer assumptions, not user errors. They act as internal documentation that disappears in production (when assertions are disabled). This improves code clarity, ensures that hidden assumptions are explicit, and strengthens maintainability without affecting runtime behavior for end users.
Add assertions to document parser and deadline assumptions
Note
Features
Milestones
LocalDate)Task list
Sample usage