[Goh Jin Yuan] iP#673
Conversation
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.
There was a problem hiding this comment.
Looks like this class declaration is empty. Was this intentional, or is this still in progress?
| class farquaadException extends Exception { | ||
| farquaadException(String msg) { super(msg); } | ||
| } | ||
|
|
||
| class unknownCommandException extends farquaadException { | ||
| unknownCommandException() { | ||
| super("I apologise, but I don't know what that means LOL"); | ||
| } | ||
| } | ||
|
|
||
| class emptyDescriptionException extends farquaadException { | ||
| emptyDescriptionException(String task) { | ||
| super("lmao the description of a " + task + " cannot be empty."); | ||
| } | ||
| } | ||
|
|
||
| class invalidIndexException extends farquaadException { | ||
| invalidIndexException() { | ||
| super("the task number is invalid."); | ||
| } | ||
| } |
There was a problem hiding this comment.
Class names should follow PascalCase, so you may want to update these.
There was a problem hiding this comment.
I second the point @SanguineChameleon made about following naming conventions
makis4n
left a comment
There was a problem hiding this comment.
LGTM 👍 . But there are definitely some points for improvement (e.g. including Javadocs and adding access modifiers to your classes).
| System.out.println("Nice! I've marked this task as done:"); | ||
| } | ||
|
|
||
| public void unmarkAsNotDone() { |
There was a problem hiding this comment.
Perhaps consider changing the method name to something a bit less confusing (e.g. markAsNotDone)?
| class farquaadException extends Exception { | ||
| farquaadException(String msg) { super(msg); } | ||
| } | ||
|
|
||
| class unknownCommandException extends farquaadException { | ||
| unknownCommandException() { | ||
| super("I apologise, but I don't know what that means LOL"); | ||
| } | ||
| } | ||
|
|
||
| class emptyDescriptionException extends farquaadException { | ||
| emptyDescriptionException(String task) { | ||
| super("lmao the description of a " + task + " cannot be empty."); | ||
| } | ||
| } | ||
|
|
||
| class invalidIndexException extends farquaadException { | ||
| invalidIndexException() { | ||
| super("the task number is invalid."); | ||
| } | ||
| } |
There was a problem hiding this comment.
I second the point @SanguineChameleon made about following naming conventions
| } | ||
| } | ||
|
|
||
| class invalidIndexException extends farquaadException { |
|
|
||
| class EmptyDescriptionException extends farquaadException { | ||
| EmptyDescriptionException(String task) { | ||
| class emptyDescriptionException extends farquaadException { |
| } | ||
| } | ||
|
|
||
| public class farquaad { |
Butanol
left a comment
There was a problem hiding this comment.
Overall readable code other than a few coding standard violations. Maybe split the classes into different files.
aliensarefake
left a comment
There was a problem hiding this comment.
Code Review - Coding Standards and Quality
Coding Standard Violations:
-
Variable Naming Convention
static final SaveFunction savefunctionshould use camelCase- Should be:
static final SaveFunction saveFunction
-
Method Naming Issues
unmarkAsNotDone()contains a confusing double negative- Consider
markAsNotDone()or simplyunmark()for clarity getIsDone()should follow boolean getter convention:isDone()
-
Static Members in Main Class
- Using
static ArrayList<Task> tasksin the main class reduces testability - Consider encapsulating in an instance-based TaskManager class for better design
- Using
Code Quality Observations:
- Good exception hierarchy with custom
FarquaadExceptionextending base Exception - Nested static classes (ToDo, Deadline, Event) work but may benefit from separate files as complexity grows
- Consider using primitive
booleanreturn type instead ofBooleanwrapper ingetIsDone()
|
Good in-code comment written |
…es and storage Currently, invalid inputs or broken invariants can pass silently, making defects harder to diagnose (e.g., null/empty user input, out-of-bounds indices, or malformed storage loads). This reduces debuggability and can allow incorrect program states to propagate before failing elsewhere. Let's add Java assertions at critical points to guard assumptions about inputs and state transitions. Specifically: * Parser: assert non-null input, non-empty command word, and valid non-negative indices after parsing where applicable. * TaskList: - add(): assert size increases by exactly one. - remove(): assert index is within bounds and size decreases by one. - get(): assert index is within bounds. * Storage.load(): assert the loaded task collection is non-null. * Command classes (e.g., Deadline, Event, Delete): assert required arguments are non-null/non-empty and constructed Task objects are non-null before mutation and save. These assertions improve fault localization during development and test runs when assertions are enabled, without changing runtime behavior in production (assertions are disabled by default). How to run with assertions: `java -ea -jar build/libs/<app>.jar` No functional changes when assertions are disabled.
The Ui class currently contains duplicated logic for repeated task list formatting in displayTaskList and displayMatchingTasks. This makes the code harder to read and more error-prone when extending or updating output formatting. Let's refactor Ui to improve code quality by: * Extracting a private helper formatTaskList() to eliminate duplication between displayTaskList and displayMatchingTasks. * Declaring the scanner field as final to enforce immutability. These changes improve code readability, reduce duplication, and make Ui easier to maintain and extend in the future without changing program behavior.
Add assertions to ensure program correctness
Merge branch 'master' into branch-A-CodeQuality
Improve code quality for readability and maintainability
Users may forget deadlines as the app does not proactively surface tasks due soon. Let's implement a `remind` command and also display reminders at app startup for tasks with deadlines within the next 3 days. Specifically: * Add RemindCommand to list deadlines due within N days (default 3). * Wire Parser to recognize the `remind` keyword. * Add Ui.displayMessage to standardize print-and-return outputs. * Update Farquaad to compute and display startup reminders in CLI and GUI (via MainWindow.setFarquaad). This improves usability by surfacing upcoming deadlines without the user having to remember to query them. be ignored, and an empty message aborts the commit.
Farquaad 🤖
Farquaad is your loyal text-based chatbot that helps you keep track of tasks, deadlines, and events —
so you don’t have to!
It is:
All you need to do is:
And best of all — it’s FREE!
✨ Features
🛠️ For Java Programmers
If you’re a Java programmer, Farquaad is also a great way to practice OOP principles, JavaFX (if you extend it), and working with file I/O.
Here’s the main method to launch it: