CodeRabbit Review#7
Conversation
There are currently no assertion statements in the code. Assertion statements define our assumptions of the program state. These assumptions are verified at runtime, allowing errors to be caught early. Let's add some assertion statements to ensure our assumptions about the program state are true.
The current code has areas for improvement in terms of code quality. Improving code quality allows for code to be better read, understood and modified by other developers. Let's *Shorten methods *Improve logical structure of code *Remove unnecessary comments *Refactor by extracting methods
Add assertions
# Conflicts: # src/main/java/serenity/Parser.java
Improve code quality
WalkthroughReplaces Duke with Serenity, introducing core bot logic, task domain, storage, parser, CLI and JavaFX GUI, and tests. Adds Checkstyle configuration and Gradle updates (JavaFX deps, main class switch, shadowJar rename). Removes old Duke entry point. Updates docs, sample data, and text UI tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as Ui (CLI)
participant Core as Serenity
participant Parser as Parser
participant Tasks as TaskList
participant Store as Storage
User->>CLI: Enter command
CLI->>Core: readCommand()
Core->>Parser: parse(input, tasks, ui, storage)
alt create (todo/deadline/event)
Parser->>Tasks: createTask(input)
Parser->>Tasks: addTask(task)
Parser->>Store: saveTask(task) or writeToFile(tasks)
Parser-->>CLI: showMessage(...)
else list/find
Parser->>Tasks: toString()/findTask()
Parser-->>CLI: showTaskList/showMessage
else mark/unmark/delete/update
Parser->>Tasks: mutate(...)
Parser->>Store: writeToFile(tasks)
Parser-->>CLI: showMessage(...)
else bye
Parser-->>CLI: showGoodbye()
end
sequenceDiagram
autonumber
actor User
participant FX as JavaFX Stage
participant MW as MainWindow
participant Core as Serenity
participant Parser as Parser
participant Tasks as TaskList
participant Store as Storage
Note over FX,MW: App start
FX->>MW: initialize() + start()
MW->>Core: welcome()
MW->>MW: render welcome dialog
User->>MW: type message + Send
MW->>Core: getResponse(input)
Core->>Parser: parseToString(input, tasks, storage)
Parser->>Tasks: operations (create/list/mark/...)
Parser->>Store: save/write as needed
Parser-->>Core: response string
Core-->>MW: response string
MW->>MW: append user + bot DialogBox
alt input == "bye"
MW->>FX: close stage
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 30
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
text-ui-test/runtest.bat (1)
10-14: Compile step misses subpackages; tests won’t buildThe current javac glob only compiles top-level sources and will skip package directories like serenity/gui.
-rem compile the code into the bin folder -javac -cp ..\src\main\java -Xlint:none -d ..\bin ..\src\main\java\*.java +REM compile all sources (including subpackages) into the bin folder +dir /s /b ..\src\main\java\*.java > sources.txt +javac -cp ..\src\main\java -Xlint:none -d ..\bin @sources.txt +if exist sources.txt del sources.txt
🧹 Nitpick comments (39)
config/checkstyle/suppressions.xml (1)
8-9: Optionally narrow the files regex to test sources only.Prevents accidental suppression if a non-test file ends with “Test.java” elsewhere.
Example:
- <suppress checks="JavadocType" files=".*Test\.java"/> - <suppress checks="MissingJavadocMethod" files=".*Test\.java"/> + <suppress checks="JavadocType" files="^src/test/java/.*Test\.java$"/> + <suppress checks="MissingJavadocMethod" files="^src/test/java/.*Test\.java$"/>text-ui-test/input.txt (1)
1-9: Happy-path coverage looks good; consider adding a few negative/edge cases.Add 1–2 invalid commands (e.g., “mark 99”, malformed deadlines) to guard parser error paths. Keep in a separate test input to avoid bloating this one.
text-ui-test/EXPECTED.TXT (1)
1-4: Freeze UI header/footer strings as constants to avoid brittle test diffs.If not already, centralize these separators and messages in Ui; keeps tests stable when branding changes.
Also applies to: 5-6, 10-13, 15-18, 20-25, 26-29, 30-33, 34-37, 38-43, 44-46
data/serenity.txt (1)
1-4: Add one future-dated and one malformed entry to a dedicated negative test fixture.Helps verify Storage error handling without risking main sample integrity.
src/main/java/serenity/SerenityException.java (2)
3-6: Clarify Javadoc: it’s an app-level exception, not “in the Serenity class.”Minor wording improvement.
Apply this diff:
-/** - * Represents a custom exception in the Serenity class. - */ +/** + * Application-level exception type for Serenity. + */
6-16: Add serialVersionUID and overloads for flexibility.Improves serialization safety and lets callers pass causes.
Apply this diff:
public class SerenityException extends Exception { + private static final long serialVersionUID = 1L; /** * Constructs a SerenityException. * * @param msg The error message of the exception. */ public SerenityException(String msg) { super(msg); } + + /** + * Constructs a SerenityException with a cause. + * + * @param msg the error message + * @param cause the underlying cause + */ + public SerenityException(String msg, Throwable cause) { + super(msg, cause); + } }src/main/resources/view/DialogBox.fxml (2)
8-18: Ensure bubbles wrap properly and don’t crowd the avatarAdd spacing and let the Label grow horizontally so long messages wrap instead of overlapping the ImageView.
-<fx:root alignment="TOP_RIGHT" +<fx:root alignment="TOP_RIGHT" spacing="10.0" @@ - <Label fx:id="dialog" text="Label" wrapText="true" minHeight="-Infinity"/> + <Label fx:id="dialog" text="Label" wrapText="true" minHeight="-Infinity" HBox.hgrow="ALWAYS" maxWidth="1.7976931348623157E308"/>
11-11: Avoid hard-coding prefWidth to improve responsivenessLet the parent/container decide width. The fixed 400px can cause awkward wrapping on wider windows.
- prefWidth="400.0"src/main/java/serenity/task/Task.java (1)
15-18: Null-safety for descriptionGuard against null descriptions to avoid surprises elsewhere (e.g., rendering).
+import java.util.Objects; @@ - public Task(String description) { - this.description = description; + public Task(String description) { + this.description = Objects.requireNonNull(description, "description"); this.isDone = false; }docs/README.md (3)
73-79: Grammar: plural agreement“Tasks … which contain”, not “contains”.
-Finds all tasks in the task list which contains the given keyword. +Finds all tasks in the task list which contain the given keyword.
60-61: Clarify unmark semanticsSay “mark as not done” to avoid confusion.
-Unmarks the task at the given index as done. +Marks the task at the given index as not done.
102-104: Tighten phrasing per LanguageTool hintImprove readability.
-- The task type is either `todo`, `deadline`, or `event` -- The description required is dependent on the task type +- The task type is one of: `todo`, `deadline`, `event` +- The required description depends on the task type.src/test/java/serenity/TodoTest.java (1)
10-18: Broaden coverage: test marked state and edge descriptions.Add tests for marked tasks (expect “[T][X] …” and “T | 1 | …”) and tricky descriptions (leading/trailing spaces, Unicode).
Example additional test:
@Test public void testMarkedAndData() { Todo t = new Todo("read book"); t.mark(); assertEquals("[T][X] read book", t.toString()); assertEquals("T | 1 | read book", t.formatData()); }src/main/java/serenity/gui/Main.java (3)
12-14: Update Javadoc branding to Serenity.-/** - * A GUI for Duke using FXML. - */ +/** + * Serenity GUI using FXML. + */
21-35: Fail fast on missing FXML and show a user-friendly error dialog.Null FXML URL or load failures currently just print a stack trace. Guard the resource and surface an Alert, then exit cleanly.
- try { - FXMLLoader fxmlLoader = new FXMLLoader(Main.class.getResource("/view/MainWindow.fxml")); + try { + var fxml = Main.class.getResource("/view/MainWindow.fxml"); + if (fxml == null) { + throw new IOException("Missing resource: /view/MainWindow.fxml"); + } + FXMLLoader fxmlLoader = new FXMLLoader(fxml); AnchorPane ap = fxmlLoader.load(); Scene scene = new Scene(ap); stage.setScene(scene); stage.setTitle("Serenity"); // inject the serenity instance - MainWindow window = fxmlLoader.<MainWindow>getController(); + MainWindow window = fxmlLoader.getController(); window.setSerenity(serenity); window.start(); stage.show(); } catch (IOException e) { - e.printStackTrace(); + var alert = new javafx.scene.control.Alert(javafx.scene.control.Alert.AlertType.ERROR); + alert.setHeaderText("Failed to load UI"); + alert.setContentText(e.getMessage()); + alert.showAndWait(); + javafx.application.Platform.exit(); }Add imports:
import javafx.application.Platform; import javafx.scene.control.Alert;
28-28: Drop redundant explicit generic on getController().- MainWindow window = fxmlLoader.<MainWindow>getController(); + MainWindow window = fxmlLoader.getController();src/main/resources/view/MainWindow.fxml (1)
16-42: Prefer anchors over absolute layout for responsive resizing; bind content width.Replace layoutX/layoutY with AnchorPane anchors and enable fitToWidth so the dialog stretches with the window.
<TextField fx:id="userInput" - layoutY="558.0" onAction="#handleUserInput" prefHeight="41.0" prefWidth="324.0" - AnchorPane.bottomAnchor="1.0" /> + AnchorPane.leftAnchor="0.0" + AnchorPane.rightAnchor="76.0" + AnchorPane.bottomAnchor="1.0" /> <Button fx:id="sendButton" - layoutX="324.0" - layoutY="558.0" mnemonicParsing="false" onAction="#handleUserInput" prefHeight="41.0" prefWidth="76.0" - text="Send" /> + text="Send" + AnchorPane.rightAnchor="0.0" + AnchorPane.bottomAnchor="1.0" /> <ScrollPane fx:id="scrollPane" hbarPolicy="NEVER" hvalue="1.0" - prefHeight="557.0" - prefWidth="400.0" - vvalue="1.0"> + vvalue="1.0" + fitToWidth="true" + AnchorPane.topAnchor="0.0" + AnchorPane.leftAnchor="0.0" + AnchorPane.rightAnchor="0.0" + AnchorPane.bottomAnchor="42.0"> <content> <VBox fx:id="dialogContainer" - prefHeight="552.0" - prefWidth="388.0" /> + prefHeight="552.0" + prefWidth="388.0" + fillWidth="true"/> </content> </ScrollPane>src/main/java/serenity/task/Event.java (1)
19-23: Validate inputs to protect persistence format.Guard against nulls and disallow the ‘|’ delimiter in fields to avoid corrupting saved data.
public Event(String description, String start, String end) { super(description); - this.start = start; - this.end = end; + if (start == null || end == null) { + throw new IllegalArgumentException("start/end must not be null"); + } + if (start.contains("|") || end.contains("|")) { + throw new IllegalArgumentException("start/end must not contain '|'"); + } + this.start = start; + this.end = end; }If preferred, we can instead escape ‘|’ on write and unescape on read; say the word and I’ll draft both sides.
src/main/java/serenity/gui/MainWindow.java (5)
28-30: Harden image resource loading to avoid NPEs.getResourceAsStream may return null at runtime; construct Images via a small helper with a null check.
- private Image userImage = new Image(this.getClass().getResourceAsStream("/images/User.png")); - private Image serenityImage = new Image(this.getClass().getResourceAsStream("/images/Serenity.png")); + private final Image userImage = loadImage("/images/User.png"); + private final Image serenityImage = loadImage("/images/Serenity.png"); + + private Image loadImage(String path) { + var is = java.util.Objects.requireNonNull( + this.getClass().getResourceAsStream(path), + "Missing resource: " + path); + return new Image(is); + }
36-39: Guard injection for robustness.Fail fast if serenity is not provided.
- public void setSerenity(Serenity s) { - serenity = s; - } + public void setSerenity(Serenity s) { + this.serenity = java.util.Objects.requireNonNull(s, "Serenity must be injected before use"); + }
45-47: Avoid possible NPE on startup.If start() is called before setSerenity(...), this will NPE.
- public void start() { - dialogContainer.getChildren().addAll(DialogBox.getSerenityDialog(serenity.welcome(), serenityImage)); - } + public void start() { + if (serenity == null) { + throw new IllegalStateException("Serenity not injected. Call setSerenity(...) before start()."); + } + dialogContainer.getChildren().add(DialogBox.getSerenityDialog(serenity.welcome(), serenityImage)); + }
49-52: Rename “Duke” to “Serenity” in Javadoc.- * Creates two dialog boxes, one echoing user input and the other containing Duke's reply and then appends them to + * Creates two dialog boxes, one echoing user input and the other containing Serenity's reply, and appends them to
54-65: Trim/ignore empty input and handle case-insensitive exit.Prevents blank message bubbles and supports “BYE/Bye”.
- private void handleUserInput() { - String input = userInput.getText(); + private void handleUserInput() { + String input = userInput.getText().trim(); + if (input.isEmpty()) { + return; + } String response = serenity.getResponse(input); dialogContainer.getChildren().addAll( DialogBox.getUserDialog(input, userImage), DialogBox.getSerenityDialog(response, serenityImage) ); userInput.clear(); - if (input.equals("bye")) { + if (input.equalsIgnoreCase("bye")) { Platform.exit(); } }src/main/java/serenity/task/Deadline.java (3)
16-17: Define shared DateTimeFormatters once.Avoid recreating formatters and keep formats consistent.
protected LocalDate date; + private static final DateTimeFormatter INPUT_FMT = DateTimeFormatter.ofPattern("dd/MM/yyyy"); + private static final DateTimeFormatter OUTPUT_FMT = DateTimeFormatter.ofPattern("dd MMM yyyy");
41-43: Reuse formatter constant.- String formatDate = date.format(DateTimeFormatter.ofPattern("dd MMM yyyy")); + String formatDate = date.format(OUTPUT_FMT);
51-54: Reuse formatter constant (save format).- String formatDate = date.format(DateTimeFormatter.ofPattern("dd MMM yyyy")); + String formatDate = date.format(OUTPUT_FMT);Confirm Storage.load/save expects this display format; otherwise consider persisting ISO-8601 (yyyy-MM-dd) and formatting only for UI.
src/main/java/serenity/gui/DialogBox.java (1)
29-35: Decouple FXML lookup and fail fast on load errors.Use DialogBox.class for resource resolution and throw a runtime exception instead of printing the stack trace.
- FXMLLoader fxmlLoader = new FXMLLoader(MainWindow.class.getResource("/view/DialogBox.fxml")); + FXMLLoader fxmlLoader = new FXMLLoader(DialogBox.class.getResource("/view/DialogBox.fxml")); fxmlLoader.setController(this); fxmlLoader.setRoot(this); fxmlLoader.load(); - } catch (IOException e) { - e.printStackTrace(); - } + } catch (IOException e) { + throw new IllegalStateException("Failed to load /view/DialogBox.fxml", e); + }src/test/java/serenity/TaskListTest.java (1)
101-110: Consistent test naming.Use lower camel case: updateTask_deadline_success.
src/main/java/serenity/Serenity.java (1)
89-91: Use the default path consistentlyMain should rely on the default constructor to avoid duplicating the file path string.
Apply this diff:
- new Serenity("./data/serenity.txt").run(); + new Serenity().run();src/main/java/serenity/task/TaskList.java (3)
109-110: Fix Javadoc: wrong return type descriptionDocstring says “Deadline” for createTodo; should be “Todo”.
Apply this diff:
- * @return Deadline with given task description. + * @return Todo with given task description.
265-272: Use StringBuilder to avoid quadratic concatenation in toStringFor long lists, repeated string concatenation is inefficient.
Apply this diff:
@Override public String toString() { - String message = "Here are the tasks in your list:"; - for (int i = 0; i < tasks.size(); i++) { - int index = i + 1; - message += "\n" + index + ". " + tasks.get(i); - } - return message; + StringBuilder sb = new StringBuilder("Here are the tasks in your list:"); + for (int i = 0; i < tasks.size(); i++) { + sb.append('\n').append(i + 1).append(". ").append(tasks.get(i)); + } + return sb.toString(); }
279-285: Use StringBuilder in formatDataSame concatenation concern as toString.
Apply this diff:
public String formatData() { - String data = ""; - for (int i = 0; i < tasks.size(); i++) { - data += tasks.get(i).formatData() + "\n"; - } - return data; + StringBuilder data = new StringBuilder(); + for (int i = 0; i < tasks.size(); i++) { + data.append(tasks.get(i).formatData()).append('\n'); + } + return data.toString(); }src/test/java/serenity/StorageTest.java (2)
3-3: Add import for assertThrows.import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows;
21-28: Avoid relying on toString() for semantic equality.Comparing toString() is brittle. Prefer asserting type and core fields (description, date, done) directly, or implement equals/hashCode for tasks and compare objects.
src/main/java/serenity/Storage.java (2)
25-34: Tighten encapsulation of filePath.Make filePath private final; it never changes after construction.
- protected String filePath; + private final String filePath;
51-52: Assertion is a no-op in production.Either remove it or convert to a runtime check with a meaningful exception.
config/checkstyle/checkstyle.xml (1)
238-239: Remove duplicate AtclauseOrder module.Defined twice; keep one to avoid duplicate checks.
- <module name="AtclauseOrder"/>(Apply to either the earlier or later occurrence, keeping a single definition.)
Also applies to: 388-390
src/main/java/serenity/Parser.java (2)
130-132: Make exit check robust to surrounding whitespace.- return input.startsWith("bye"); + return input != null && input.trim().equals("bye");
33-67: Minor: reduce duplication between parse and parseToString.Consider extracting a common command dispatcher that returns a result object (message + side effects flag), used by both methods.
Also applies to: 86-117
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (3)
docs/Ui.pngis excluded by!**/*.pngsrc/main/resources/images/Serenity.pngis excluded by!**/*.pngsrc/main/resources/images/User.pngis excluded by!**/*.png
📒 Files selected for processing (28)
build.gradle(3 hunks)config/checkstyle/checkstyle.xml(1 hunks)config/checkstyle/suppressions.xml(1 hunks)data/serenity.txt(1 hunks)docs/README.md(1 hunks)src/main/java/Duke.java(0 hunks)src/main/java/serenity/Parser.java(1 hunks)src/main/java/serenity/Serenity.java(1 hunks)src/main/java/serenity/SerenityException.java(1 hunks)src/main/java/serenity/Storage.java(1 hunks)src/main/java/serenity/Ui.java(1 hunks)src/main/java/serenity/gui/DialogBox.java(1 hunks)src/main/java/serenity/gui/Launcher.java(1 hunks)src/main/java/serenity/gui/Main.java(1 hunks)src/main/java/serenity/gui/MainWindow.java(1 hunks)src/main/java/serenity/task/Deadline.java(1 hunks)src/main/java/serenity/task/Event.java(1 hunks)src/main/java/serenity/task/Task.java(1 hunks)src/main/java/serenity/task/TaskList.java(1 hunks)src/main/java/serenity/task/Todo.java(1 hunks)src/main/resources/view/DialogBox.fxml(1 hunks)src/main/resources/view/MainWindow.fxml(1 hunks)src/test/java/serenity/StorageTest.java(1 hunks)src/test/java/serenity/TaskListTest.java(1 hunks)src/test/java/serenity/TodoTest.java(1 hunks)text-ui-test/EXPECTED.TXT(1 hunks)text-ui-test/input.txt(1 hunks)text-ui-test/runtest.bat(1 hunks)
💤 Files with no reviewable changes (1)
- src/main/java/Duke.java
🧰 Additional context used
🪛 LanguageTool
docs/README.md
[grammar] ~102-~102: There might be a mistake here.
Context: ...k type is either todo, deadline, or event - The description required is dependent on...
(QB_NEW_EN)
text-ui-test/EXPECTED.TXT
[grammar] ~2-~2: There might be a mistake here.
Context: ...______________________ Hi, I'm Serenity! What can I do for you? _________________...
(QB_NEW_EN)
[grammar] ~6-~6: There might be a mistake here.
Context: ...__________ Got it. I've added this task: [T][ ] read book Now you have 1 task in ...
(QB_NEW_EN)
[grammar] ~11-~11: There might be a mistake here.
Context: ...__________ Got it. I've added this task: [D][ ] return book (by: Sunday) Now you ...
(QB_NEW_EN)
[grammar] ~16-~16: There might be a mistake here.
Context: ...__________ Got it. I've added this task: [E][ ] project meeting (from: Mon 2pm to...
(QB_NEW_EN)
[grammar] ~17-~17: There might be a mistake here.
Context: ... project meeting (from: Mon 2pm to: 4pm) Now you have 3 tasks in the list. ______...
(QB_NEW_EN)
[grammar] ~21-~21: There might be a mistake here.
Context: ..._______ Here are the tasks in your list: 1. [T][ ] read book 2. [D][ ] return book (...
(QB_NEW_EN)
[grammar] ~22-~22: There might be a mistake here.
Context: ... tasks in your list: 1. [T][ ] read book 2. [D][ ] return book (by: Sunday) 3. [E][ ...
(QB_NEW_EN)
[grammar] ~23-~23: There might be a mistake here.
Context: ... book 2. [D][ ] return book (by: Sunday) 3. [E][ ] project meeting (from: Mon 2pm to...
(QB_NEW_EN)
[grammar] ~27-~27: There might be a mistake here.
Context: ...___ Nice! I've marked this task as done: [T][X] read book _______________________...
(QB_NEW_EN)
[grammar] ~31-~31: There might be a mistake here.
Context: ...___ Nice! I've marked this task as done: [E][X] project meeting (from: Mon 2pm to...
(QB_NEW_EN)
[grammar] ~35-~35: There might be a mistake here.
Context: ..., I've marked this task as not done yet: [T][ ] read book _______________________...
(QB_NEW_EN)
[grammar] ~39-~39: There might be a mistake here.
Context: ..._______ Here are the tasks in your list: 1. [T][ ] read book 2. [D][ ] return book (...
(QB_NEW_EN)
[grammar] ~40-~40: There might be a mistake here.
Context: ... tasks in your list: 1. [T][ ] read book 2. [D][ ] return book (by: Sunday) 3. [E][X...
(QB_NEW_EN)
[grammar] ~41-~41: There might be a mistake here.
Context: ... book 2. [D][ ] return book (by: Sunday) 3. [E][X] project meeting (from: Mon 2pm to...
(QB_NEW_EN)
[style] ~45-~45: Using many exclamation marks might seem excessive (in this case: 4 exclamation marks for a text that’s 841 characters long)
Context: ...____ Goodbye. Hope to see you again soon! _______________________________________...
(EN_EXCESSIVE_EXCLAMATION)
🔇 Additional comments (8)
text-ui-test/EXPECTED.TXT (1)
1-46: EXPECTED matches the provided input; no functional issues spotted.Output lines and task counts align with the commands sequence.
data/serenity.txt (1)
1-4: LGTM: sample data formats look consistent with typical iP storage.Date/times and done flags look coherent.
src/main/java/serenity/gui/Launcher.java (1)
9-11: LGTM: minimal, correct JavaFX launcherWorks around JavaFX classpath issues cleanly.
src/test/java/serenity/TodoTest.java (1)
10-18: Solid, focused assertions for Todo’s string and persistence formats.Both expectations match the current Todo implementation and provide good guardrails.
src/main/java/serenity/task/Todo.java (1)
22-25: Todo string and data formats are correct and consistent with tests.Also applies to: 32-35
src/test/java/serenity/StorageTest.java (2)
12-18: LGTM: happy path (Todo) is covered clearly.
30-37: LGTM: event deserialization test reads well and is precise.config/checkstyle/checkstyle.xml (1)
1-434: Verify Checkstyle config loads with Gradle 10.2 toolVersion
Ensure JAVA_HOME points to a valid JDK (e.g. Java 17), then run:export JAVA_HOME=/path/to/valid/jdk ./gradlew --no-daemon checkstyleMain checkstyleTest -iConfirm the config parses without errors.
| String javaFxVersion = '17.0.7' | ||
|
|
||
| implementation group: 'org.openjfx', name: 'javafx-base', version: javaFxVersion, classifier: 'win' | ||
| implementation group: 'org.openjfx', name: 'javafx-base', version: javaFxVersion, classifier: 'mac' | ||
| implementation group: 'org.openjfx', name: 'javafx-base', version: javaFxVersion, classifier: 'linux' | ||
| implementation group: 'org.openjfx', name: 'javafx-controls', version: javaFxVersion, classifier: 'win' | ||
| implementation group: 'org.openjfx', name: 'javafx-controls', version: javaFxVersion, classifier: 'mac' | ||
| implementation group: 'org.openjfx', name: 'javafx-controls', version: javaFxVersion, classifier: 'linux' | ||
| implementation group: 'org.openjfx', name: 'javafx-fxml', version: javaFxVersion, classifier: 'win' | ||
| implementation group: 'org.openjfx', name: 'javafx-fxml', version: javaFxVersion, classifier: 'mac' | ||
| implementation group: 'org.openjfx', name: 'javafx-fxml', version: javaFxVersion, classifier: 'linux' | ||
| implementation group: 'org.openjfx', name: 'javafx-graphics', version: javaFxVersion, classifier: 'win' | ||
| implementation group: 'org.openjfx', name: 'javafx-graphics', version: javaFxVersion, classifier: 'mac' | ||
| implementation group: 'org.openjfx', name: 'javafx-graphics', version: javaFxVersion, classifier: 'linux' |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid bundling all OS JavaFX artifacts; use the OpenJFX Gradle plugin.
Shading win/mac/linux variants together bloats the JAR and can cause native clashes. Let the plugin resolve platform-specific artifacts at build/run time.
- String javaFxVersion = '17.0.7'
-
- implementation group: 'org.openjfx', name: 'javafx-base', version: javaFxVersion, classifier: 'win'
- implementation group: 'org.openjfx', name: 'javafx-base', version: javaFxVersion, classifier: 'mac'
- implementation group: 'org.openjfx', name: 'javafx-base', version: javaFxVersion, classifier: 'linux'
- implementation group: 'org.openjfx', name: 'javafx-controls', version: javaFxVersion, classifier: 'win'
- implementation group: 'org.openjfx', name: 'javafx-controls', version: javaFxVersion, classifier: 'mac'
- implementation group: 'org.openjfx', name: 'javafx-controls', version: javaFxVersion, classifier: 'linux'
- implementation group: 'org.openjfx', name: 'javafx-fxml', version: javaFxVersion, classifier: 'win'
- implementation group: 'org.openjfx', name: 'javafx-fxml', version: javaFxVersion, classifier: 'mac'
- implementation group: 'org.openjfx', name: 'javafx-fxml', version: javaFxVersion, classifier: 'linux'
- implementation group: 'org.openjfx', name: 'javafx-graphics', version: javaFxVersion, classifier: 'win'
- implementation group: 'org.openjfx', name: 'javafx-graphics', version: javaFxVersion, classifier: 'mac'
- implementation group: 'org.openjfx', name: 'javafx-graphics', version: javaFxVersion, classifier: 'linux'
+ // JavaFX dependencies are managed by the javafx {} block below-plugins {
+plugins {
id 'java'
id 'application'
id 'com.github.johnrengelman.shadow' version '7.1.2'
id 'checkstyle'
+ id 'org.openjfx.javafxplugin' version '0.0.14'
}Add near the application block:
javafx {
version = '17.0.7'
modules = ['javafx.controls', 'javafx.fxml']
}Optional: pin toolchain to 17 for consistency with JavaFX 17.
java {
toolchain { languageVersion = JavaLanguageVersion.of(17) }
}🤖 Prompt for AI Agents
In build.gradle around lines 15-28, remove the per-OS javafx implementation
dependencies (the win/mac/linux classifier lines) and instead apply the OpenJFX
Gradle plugin and declare the javafx configuration in the application block so
platform artifacts are resolved at build/run time; add a javafx block setting
version = '17.0.7' and modules = ['javafx.controls','javafx.fxml'], and
optionally pin the Java toolchain to Java 17 (java.toolchain languageVersion =
17) for consistency with JavaFX 17.
| shadowJar { | ||
| archiveBaseName = "duke" | ||
| archiveBaseName = "serenity" | ||
| archiveClassifier = null | ||
| archiveFileName = 'serenity.jar' | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Set Main-Class in shaded JAR manifest.
Ensures java -jar serenity.jar runs without extra args.
shadowJar {
archiveBaseName = "serenity"
archiveClassifier = null
archiveFileName = 'serenity.jar'
+ manifest {
+ attributes 'Main-Class': 'serenity.gui.Launcher'
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| shadowJar { | |
| archiveBaseName = "duke" | |
| archiveBaseName = "serenity" | |
| archiveClassifier = null | |
| archiveFileName = 'serenity.jar' | |
| } | |
| shadowJar { | |
| archiveBaseName = "serenity" | |
| archiveClassifier = null | |
| archiveFileName = 'serenity.jar' | |
| manifest { | |
| attributes 'Main-Class': 'serenity.gui.Launcher' | |
| } | |
| } |
🤖 Prompt for AI Agents
In build.gradle around lines 49–53, the shadowJar block doesn't set a Main-Class
in the JAR manifest so running "java -jar serenity.jar" fails; update the
shadowJar configuration to add a manifest entry "Main-Class" pointing to your
application's fully-qualified main class (e.g. com.yourorg.Main), ensuring the
application/mainClassName (or application.plugin) is set consistently, and
re-run the shadowJar task so the produced serenity.jar contains that Main-Class
manifest attribute.
| <property name="format" value="((//.*)|(\*.*))FIXME" /> | ||
| <property name="message" value='TODO is preferred to FIXME."' /> | ||
| </module> |
There was a problem hiding this comment.
Fix trailing quote in RegexpSingleline message.
There’s an extra double quote at the end.
- <property name="message" value='TODO is preferred to FIXME."' />
+ <property name="message" value="TODO is preferred to FIXME." />📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <property name="format" value="((//.*)|(\*.*))FIXME" /> | |
| <property name="message" value='TODO is preferred to FIXME."' /> | |
| </module> | |
| <property name="format" value="((//.*)|(\*.*))FIXME" /> | |
| <property name="message" value="TODO is preferred to FIXME." /> | |
| </module> |
🤖 Prompt for AI Agents
In config/checkstyle/checkstyle.xml around lines 24-26, the message attribute
value contains an extra trailing double quote (value='TODO is preferred to
FIXME."') — remove the stray double quote so the attribute reads value='TODO is
preferred to FIXME.' and ensure the XML attribute quoting remains consistent.
| <suppress checks="JavadocType" files=".*Test\.java"/> | ||
| <suppress checks="MissingJavadocMethodCheck" files=".*Test\.java"/> |
There was a problem hiding this comment.
Fix Checkstyle check name: use MissingJavadocMethod (without 'Check').
In SuppressionFilter, the check name should be the module’s simple name (e.g., MissingJavadocMethod), not the class name with the Check suffix.
Apply this diff:
<suppress checks="JavadocType" files=".*Test\.java"/>
- <suppress checks="MissingJavadocMethodCheck" files=".*Test\.java"/>
+ <suppress checks="MissingJavadocMethod" files=".*Test\.java"/>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <suppress checks="JavadocType" files=".*Test\.java"/> | |
| <suppress checks="MissingJavadocMethodCheck" files=".*Test\.java"/> | |
| <suppress checks="JavadocType" files=".*Test\.java"/> | |
| <suppress checks="MissingJavadocMethod" files=".*Test\.java"/> |
🤖 Prompt for AI Agents
In config/checkstyle/suppressions.xml around lines 8 to 9, the suppression uses
the incorrect Checkstyle module name "MissingJavadocMethodCheck"; update that
value to the module’s simple name "MissingJavadocMethod" (i.e., change the
checks attribute on the second suppress element from MissingJavadocMethodCheck
to MissingJavadocMethod) so SuppressionFilter matches correctly.
| Adds a deadline task to the task list. | ||
|
|
||
| Format: `deadline DESCRIPTION /from START /to END` | ||
|
|
||
| Examples: | ||
| - `event project meeting /from Monday 2pm /to 4pm` |
There was a problem hiding this comment.
Fix command type: this section is about event, not deadline
Header text and format currently reference “deadline”.
-Adds a deadline task to the task list.
+Adds an event task to the task list.
@@
-Format: `deadline DESCRIPTION /from START /to END`
+Format: `event DESCRIPTION /from START /to END`📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Adds a deadline task to the task list. | |
| Format: `deadline DESCRIPTION /from START /to END` | |
| Examples: | |
| - `event project meeting /from Monday 2pm /to 4pm` | |
| Adds an event task to the task list. | |
| Format: `event DESCRIPTION /from START /to END` | |
| Examples: | |
| - `event project meeting /from Monday 2pm /to 4pm` |
🤖 Prompt for AI Agents
In docs/README.md around lines 31 to 36, the header and format incorrectly
reference "deadline" while the examples and section describe an event command;
change the header text and command format to refer to "event" (e.g., "Adds an
event task to the task list." and "Format: `event DESCRIPTION /from START /to
END`") and update any example phrasing if needed to consistently use "event"
instead of "deadline".
| @Test | ||
| public void changeStatus_missingIndex_exceptionThrown() { | ||
| ArrayList<Task> tasks = new ArrayList<>(); | ||
| TaskList taskList = new TaskList(tasks); | ||
| Todo task = new Todo("read book"); | ||
| taskList.addTask(task); | ||
| try { | ||
| taskList.changeStatus("mark "); | ||
| } catch (SerenityException e) { | ||
| assertEquals("Error: Missing index.", | ||
| e.getMessage()); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor to assertThrows (missing index).
- try {
- taskList.changeStatus("mark ");
- } catch (SerenityException e) {
- assertEquals("Error: Missing index.",
- e.getMessage());
- }
+ SerenityException e = assertThrows(SerenityException.class,
+ () -> taskList.changeStatus("mark "));
+ assertEquals("Error: Missing index.", e.getMessage());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Test | |
| public void changeStatus_missingIndex_exceptionThrown() { | |
| ArrayList<Task> tasks = new ArrayList<>(); | |
| TaskList taskList = new TaskList(tasks); | |
| Todo task = new Todo("read book"); | |
| taskList.addTask(task); | |
| try { | |
| taskList.changeStatus("mark "); | |
| } catch (SerenityException e) { | |
| assertEquals("Error: Missing index.", | |
| e.getMessage()); | |
| } | |
| } | |
| @Test | |
| public void changeStatus_missingIndex_exceptionThrown() { | |
| ArrayList<Task> tasks = new ArrayList<>(); | |
| TaskList taskList = new TaskList(tasks); | |
| Todo task = new Todo("read book"); | |
| taskList.addTask(task); | |
| SerenityException e = assertThrows(SerenityException.class, | |
| () -> taskList.changeStatus("mark ")); | |
| assertEquals("Error: Missing index.", e.getMessage()); | |
| } |
🤖 Prompt for AI Agents
In src/test/java/serenity/TaskListTest.java around lines 62 to 74, the test
currently uses a try/catch to verify that calling taskList.changeStatus("mark ")
throws a SerenityException for a missing index; replace this pattern with
JUnit's assertThrows: call assertThrows(SerenityException.class, () ->
taskList.changeStatus("mark ")), capture the thrown exception, and assert its
message equals "Error: Missing index."; remove the try/catch and ensure imports
for assertThrows are available.
| @Test | ||
| public void changeStatus_invalidIndex_exceptionThrown() { | ||
| ArrayList<Task> tasks = new ArrayList<>(); | ||
| TaskList taskList = new TaskList(tasks); | ||
| Todo task = new Todo("read book"); | ||
| taskList.addTask(task); | ||
| try { | ||
| taskList.changeStatus("mark 2"); | ||
| } catch (SerenityException e) { | ||
| assertEquals("Error: Index is out of bounds.", | ||
| e.getMessage()); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor to assertThrows (invalid index).
- try {
- taskList.changeStatus("mark 2");
- } catch (SerenityException e) {
- assertEquals("Error: Index is out of bounds.",
- e.getMessage());
- }
+ SerenityException e = assertThrows(SerenityException.class,
+ () -> taskList.changeStatus("mark 2"));
+ assertEquals("Error: Index is out of bounds.", e.getMessage());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Test | |
| public void changeStatus_invalidIndex_exceptionThrown() { | |
| ArrayList<Task> tasks = new ArrayList<>(); | |
| TaskList taskList = new TaskList(tasks); | |
| Todo task = new Todo("read book"); | |
| taskList.addTask(task); | |
| try { | |
| taskList.changeStatus("mark 2"); | |
| } catch (SerenityException e) { | |
| assertEquals("Error: Index is out of bounds.", | |
| e.getMessage()); | |
| } | |
| } | |
| @Test | |
| public void changeStatus_invalidIndex_exceptionThrown() { | |
| ArrayList<Task> tasks = new ArrayList<>(); | |
| TaskList taskList = new TaskList(tasks); | |
| Todo task = new Todo("read book"); | |
| taskList.addTask(task); | |
| SerenityException e = assertThrows(SerenityException.class, | |
| () -> taskList.changeStatus("mark 2")); | |
| assertEquals("Error: Index is out of bounds.", e.getMessage()); | |
| } |
🤖 Prompt for AI Agents
In src/test/java/serenity/TaskListTest.java around lines 76 to 88, the test for
an invalid index currently uses a try/catch to assert the SerenityException;
refactor it to use JUnit's assertThrows to make the test clearer and fail
correctly on no exception. Replace the try/catch with a call to
assertThrows(SerenityException.class, () -> taskList.changeStatus("mark 2")) and
capture the thrown exception to assert its message equals "Error: Index is out
of bounds."; ensure the test imports static
org.junit.jupiter.api.Assertions.assertThrows (and assertEquals if not already).
| @Test | ||
| public void updateTask_missingIndex_exceptionThrown() { | ||
| ArrayList<Task> tasks = new ArrayList<>(); | ||
| TaskList taskList = new TaskList(tasks); | ||
| Todo task = new Todo("read book"); | ||
| taskList.addTask(task); | ||
| try { | ||
| taskList.updateTask("update /update todo read English Book"); | ||
| } catch (SerenityException e) { | ||
| assertEquals("Error: Missing index.", | ||
| e.getMessage()); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor to assertThrows (update missing index).
- try {
- taskList.updateTask("update /update todo read English Book");
- } catch (SerenityException e) {
- assertEquals("Error: Missing index.",
- e.getMessage());
- }
+ SerenityException e = assertThrows(SerenityException.class,
+ () -> taskList.updateTask("update /update todo read English Book"));
+ assertEquals("Error: Missing index.", e.getMessage());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Test | |
| public void updateTask_missingIndex_exceptionThrown() { | |
| ArrayList<Task> tasks = new ArrayList<>(); | |
| TaskList taskList = new TaskList(tasks); | |
| Todo task = new Todo("read book"); | |
| taskList.addTask(task); | |
| try { | |
| taskList.updateTask("update /update todo read English Book"); | |
| } catch (SerenityException e) { | |
| assertEquals("Error: Missing index.", | |
| e.getMessage()); | |
| } | |
| } | |
| @Test | |
| public void updateTask_missingIndex_exceptionThrown() { | |
| ArrayList<Task> tasks = new ArrayList<>(); | |
| TaskList taskList = new TaskList(tasks); | |
| Todo task = new Todo("read book"); | |
| taskList.addTask(task); | |
| - try { | |
| - taskList.updateTask("update /update todo read English Book"); | |
| - } catch (SerenityException e) { | |
| - assertEquals("Error: Missing index.", | |
| - e.getMessage()); | |
| SerenityException e = assertThrows(SerenityException.class, | |
| () -> taskList.updateTask("update /update todo read English Book")); | |
| assertEquals("Error: Missing index.", e.getMessage()); | |
| } |
🤖 Prompt for AI Agents
In src/test/java/serenity/TaskListTest.java around lines 123 to 135, the test
for updateTask_missingIndex currently uses a try/catch to verify the
SerenityException; replace this with JUnit's assertThrows to make the test
clearer and more idiomatic: call assertThrows(SerenityException.class, () ->
taskList.updateTask("update /update todo read English Book")) and assert the
exception message equals "Error: Missing index." using the returned exception's
getMessage(), removing the try/catch block and the manual fail path.
| @Test | ||
| public void updateTask_invalidIndex_exceptionThrown() { | ||
| ArrayList<Task> tasks = new ArrayList<>(); | ||
| TaskList taskList = new TaskList(tasks); | ||
| Todo task = new Todo("read book"); | ||
| taskList.addTask(task); | ||
| try { | ||
| taskList.updateTask("update 2 /update todo read English Book"); | ||
| } catch (SerenityException e) { | ||
| assertEquals("Error: Index is out of bounds.", | ||
| e.getMessage()); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor to assertThrows (update invalid index).
- try {
- taskList.updateTask("update 2 /update todo read English Book");
- } catch (SerenityException e) {
- assertEquals("Error: Index is out of bounds.",
- e.getMessage());
- }
+ SerenityException e = assertThrows(SerenityException.class,
+ () -> taskList.updateTask("update 2 /update todo read English Book"));
+ assertEquals("Error: Index is out of bounds.", e.getMessage());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Test | |
| public void updateTask_invalidIndex_exceptionThrown() { | |
| ArrayList<Task> tasks = new ArrayList<>(); | |
| TaskList taskList = new TaskList(tasks); | |
| Todo task = new Todo("read book"); | |
| taskList.addTask(task); | |
| try { | |
| taskList.updateTask("update 2 /update todo read English Book"); | |
| } catch (SerenityException e) { | |
| assertEquals("Error: Index is out of bounds.", | |
| e.getMessage()); | |
| } | |
| } | |
| @Test | |
| public void updateTask_invalidIndex_exceptionThrown() { | |
| ArrayList<Task> tasks = new ArrayList<>(); | |
| TaskList taskList = new TaskList(tasks); | |
| Todo task = new Todo("read book"); | |
| taskList.addTask(task); | |
| SerenityException e = assertThrows(SerenityException.class, | |
| () -> taskList.updateTask("update 2 /update todo read English Book")); | |
| assertEquals("Error: Index is out of bounds.", e.getMessage()); | |
| } |
🤖 Prompt for AI Agents
In src/test/java/serenity/TaskListTest.java around lines 137 to 149, the test
uses a try-catch to verify an invalid-index SerenityException; replace this with
JUnit's assertThrows to make the test clearer and fail on no exception: call
assertThrows(SerenityException.class, () -> taskList.updateTask("update 2
/update todo read English Book")) and then assertEquals the expected message
against the thrown exception's getMessage(); remove the try-catch and any
unreachable code so the test fails if no exception is thrown.
|
|
||
| REM run the program, feed commands from input.txt file and redirect the output to the ACTUAL.TXT | ||
| java -classpath ..\bin Duke < input.txt > ACTUAL.TXT | ||
| java -classpath ..\bin Serenity < input.txt > ACTUAL.TXT |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Run the correct fully-qualified main class
Serenity is packaged; invoking the simple name will fail with “Could not find or load main class”.
-java -classpath ..\bin Serenity < input.txt > ACTUAL.TXT
+java -classpath ..\bin serenity.Serenity < input.txt > ACTUAL.TXTVerification (checks class/package and presence of a main):
🏁 Script executed:
#!/bin/bash
# Verify Serenity main class is packaged and has a main method
rg -nC2 '^\s*package\s+serenity\b' src/main/java/serenity/Serenity.java
rg -nC1 'public\s+static\s+void\s+main\s*\(' src/main/java/serenity/Serenity.javaLength of output: 321
Use fully-qualified class name in runtest.bat
Replace the invocation on line 18 of text-ui-test/runtest.bat:
-java -classpath ..\bin Serenity < input.txt > ACTUAL.TXT
+java -classpath ..\bin serenity.Serenity < input.txt > ACTUAL.TXTThis matches package serenity; and the public static void main(String[] args) in src/main/java/serenity/Serenity.java.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| java -classpath ..\bin Serenity < input.txt > ACTUAL.TXT | |
| java -classpath ..\bin serenity.Serenity < input.txt > ACTUAL.TXT |
🤖 Prompt for AI Agents
In text-ui-test/runtest.bat around line 18, replace the invocation that calls
the class without its package with the fully-qualified class name; specifically
update the java command to invoke serenity.Serenity (matching package serenity
and the main class Serenity) so it runs: java -classpath ..\bin
serenity.Serenity < input.txt > ACTUAL.TXT.
1 participant
Summary by CodeRabbit