[chia-yh] iP #47
Open
[chia-yh] iP #47
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Rename Duke.java to Book.java Update Book.java
Modify Task.java into an abstract class Add ToDo.java, Deadline.java, Event.java extending from Task.java Modify Book.java to handle ToDo.java, Deadline.java, Event.java
Add automated testing of text UIs with runtest.bat, input.txt, EXPECTED.TXT Fix print errors in Book.java, Event.java
Add checked exceptions to handle invalid or incomplete inputs with IncompleteInputException.java and InvalidInputException.java Update Book.java to handle exceptions Update README.md to credit Kaomoji used
Add support for deleting tasks from the list in Book.java Removed Kaomoji due to display problems
Book task list is not saved on exit and is empty on startup. Users lose all tracked tasks in task list on exit, ability to save and load tasks tracked in the task list allows users to access their tasks across multiple sessions. Let's update Book to save tasks whenever task list changes and load task list on startup using a file. * Update Book.java, Task.java, ToDo.java, Deadline.java, Event.java to support function to save task list * Update EXPECTED.TXT for automated text UI testing * Update .gitignore to ignore save folder and file
Deadlines, Events store dates as String, allow for any text to be stored. Deadlines, Events treat any String as Date/Time, parsing String as LocalDateTime enforces storing of valid Date/Time, allows for formatting and meaningful use of stored value. Let's update Book to store Date/Time as LocalDateTime instead of String. * Update Book.java, Deadline.java, Event.java, Task.java to support storage of Date/Time as LocalDateTime
Merge Level-7 onto master
Resolve conflicts between Level-7 and Level-8 with implementation of LocalDateTime
* Fix issues with save and load with Date/Time from merging Level-7, Level-8 to master * Update input.txt, EXPECTED.TXT for automated testing of text ui with Date/Time * Update README.md formatting
Book class handles nearly all functionality. With all functionality in a single class, code is harder to maintain and comprehend. Extracting related code into different classes allows specific functions to be confined within a single class, which is easier to maintain. Let's extract the following classes: * Ui: handle user interactions * Storage: handle loading tasks from file and saving tasks to file * Parser: handle interpretation of commands * TaskList: handle task list operations Let's extract the following Command classes to handle command execution: * Command: abstract class from which other Command classes inherit to aid command execution * AddCommand: handle task list operation to add tasks * DeleteCommand: handle task list operation to delete tasks * MarkCommand: handle task list operation to mark tasks as complete * UnmarkCommand: handle task list operation to mark tasks as incomplete * ExitCommand: handle exit from Book Reorganize exceptions into the following: * BookException: Exception class for Book from which other Book exceptions inherit * IncompleteInputException * InvalidFormatException * InvalidInputException * LoadException * SaveException
No organization of classes into packages. Hard to manage classes with related functionality without packages. Let's divide related classes into packages, * book: general functionality for Book * book.command: handle execution of commands * book.exception: exceptions for Book * book.task: models for tasks Packages allow for easier management of related classes and prevents name conflicts.
No gradle, checkstyle support No automation of tasks, checkstyle to enforce coding standard Adding gradle allows automation of some of the build tasks, adding checkstyle plugin allows for detection of coding standard violation Let's set up gradle with the add-gradle-support branch and implement the checkstyle plugin with checkstyle.xml, suppressions.xml from github.com/se-edu/addressbook-level3/tree/master/config/checkstyle to enforce the coding standard for the project Using checkstyle is preferable to catch coding standard violations that may be missed otherwise Update README.md to include credits for checkstyle.xml, suppressions.xml Update import statements to fulfill requirements of coding standard
Text ui testing done with text-ui-test Text ui testing tests expected output for given input, may not detect some bugs Automated testing with JUnit allows for unit testing to ensure individual units function correctly Let's add unit tests for the public methods of several classes, * DeadlineTest: unit tests for Deadline class * EventTest: unit tests for Event class * ToDoTest: unit tests for ToDo class Unit tests are preferable as they allow testing of individual units to detect bugs
Modify build.gradle to support ability to package Book as a JAR file for ease of distribution Modify Ui.java to fix typo
No JavaDoc comments exist. Lack of documentation hinders understanding of Book. Adding JavaDoc comments aids comprehension of Book by allowing for generation of explanatory tooltips and API documentation in HTML format. Let's add JavaDoc comments to non-private classes/methods, and non-trivial private methods.
Let's comply with the coding standard by fixing any violations to ensure the quality of the code.
Users of Book have no way of locating a particular task. Function to locate tasks easily would improve usability of book. Let's update Book to allow users to search for tasks with a keyword, * FindCommand: implementation of a Command for listing Tasks which match a keyword * Parser: update to parse input "find <keyword>" * TaskList: update to return representation of all Tasks matching keyword * Ui: update to print output from "find <keyword>" * Task: update to provide keyword matching functionality
Merge A-JavaDoc onto master
Merge A-CodingStandard onto master
Merge Level-9 onto master
Add JavaDoc for FindCommand and related new methods Update text-ui-test with additional tests and updated format
Book uses text-based UI. Text-based UI is not really user-friendly, users could run into issues with using Book. Let's add a GUI to Duke with JavaFX to improve usability, * Add Gui.java, DialogBox.java under book.gui to handle GUI for Book * Update Ui.java, book.command package to interact with Gui.java instead of printing to System.out * Update Book.java to launch GUI instead * Modify book.task package, TaskList to fit GUI interaction better * Add Book_Icon.png, User_Icon.png for GUI * Update JavaDoc for relevant methods * Update build.gradle for JavaFX * Update tests for book.task package
Merge Level-10 onto master
jweeyh
reviewed
Feb 2, 2023
| /** | ||
| * Class that handles the GUI for user interaction. | ||
| */ | ||
| public class Gui extends Application { |
src/main/java/book/Book.java
Outdated
| public String parseAndReturn(String userInput) { | ||
| try { | ||
| Command command = Parser.parse(userInput); | ||
| String returnFromCommand = command.execute(this.storage, this.list, this.ui); |
There was a problem hiding this comment.
Should this be called something like commandOutput instead?
| /** | ||
| * Main class of {@code Book}. | ||
| */ | ||
| public class Book { |
There was a problem hiding this comment.
Really excellent effort! It's good you've managed to document all your code clearly and managed to follow the coding standards consistently through all your classes!
src/main/java/book/Parser.java
Outdated
Comment on lines
42
to
62
| switch (inputs[0]) { | ||
| case "bye": | ||
| return new ExitCommand(); | ||
| case "list": | ||
| return new ListCommand(); | ||
| case "mark": | ||
| return new MarkCommand(Integer.parseInt(inputs[1])); | ||
| case "unmark": | ||
| return new UnmarkCommand(Integer.parseInt(inputs[1])); | ||
| case "delete": | ||
| return new DeleteCommand(Integer.parseInt(inputs[1])); | ||
| case "find": | ||
| return new FindCommand(inputs[1]); | ||
| case "todo": | ||
| return new AddCommand(new ToDo(inputs[1])); | ||
| case "deadline": | ||
| String[] deadlineDetails = inputs[1].split("/by ", 2); | ||
| return new AddCommand(new Deadline(deadlineDetails[0], | ||
| parseDateTime(deadlineDetails[1]))); | ||
| case "event": | ||
| String[] eventDetails = inputs[1].split("/from | /to ", 3); |
noahxinjie
reviewed
Feb 2, 2023
src/main/java/book/TaskList.java
Outdated
Comment on lines
66
to
69
| * Returns the {@code String} representation of {@code Task}s matching the given {@code String} | ||
| * keyword in {@code TaskList}. | ||
| * @param keyword {@code String} keyword to match {@code Task}s in {@code TaskList}. | ||
| * @return {@code String} representation of {@code Task}s matching the given {@code String} |
There was a problem hiding this comment.
Perhaps it might be better to leave an empty line between the description and parameter section of your JavaDocs?
| */ | ||
| public class Ui { | ||
| /** Indentation. */ | ||
| private static final String INDENT = " "; |
There was a problem hiding this comment.
I like that you put this static final variable in all caps :)
src/main/java/book/TaskList.java
Outdated
| * @return {@code String} representation of {@code Task}s matching the given {@code String} | ||
| * keyword in {@code TaskList}. | ||
| */ | ||
| public String matchingTasks(String keyword) { |
There was a problem hiding this comment.
Perhaps it might better to use a verb for your method here; maybe findMatchingTasks?
No use of assert to document important assumptions. Lack of assert statements to verify assumptions about program state during runtime, no mechanism to stop execution early due to possible bugs for safety. Adding assertions allow verification of assumptions about program state during runtime to allow termination in the case of possible bugs for safety. Let's add assert statements to document important assumptions that should hold, * Storage.java: assert that there is an associated save directory File and save File on instantiation * Gui.java: assert that there is an associate Book on start()
Code quality not examined recently. Critically examining code quality and refactoring improves internal structure, pays off technical debt accumulated. Let's critically examine the code and refactor to improve code quality where necessary.
Add assert statements to Gui.java to verify existence of image resources
Merge branch-A-Assertions onto master
Merge branch-A-CodeQuality onto master
No Continuous Integration (CI) set up for automated integration, building, testing after each code change. Lack of CI for automated integration, building, testing after each code change may cause bugs to be found slower, software quality to suffer. Use GitHub Actions to set up CI to allow for automated integration, building, testing after each code cahnge with custom workflow. Let's, * Add gradle.yml to ip\.github\workflows * Update README.md to credit source of gradle.yml
Update the formatting for README.md credits
Update README.md to fix formatting for credits
No function to sort items in Book. Lack of sorting function would cause tasks stored in Book to gradually clutter up over time. Adding a sorting function would allow tasks to be organised as they are added to Book, which would allow specific tasks to be found easier. Let's add the ability to sort Tasks in Book, * Parser.java: update with to parse command for SortCommand * TaskList.java: update to support new SortCommand * Ui.java: update to support UI output for new SortCommand * SortCommand.java: new subclass of Command to support sorting * Task.java: update to extend Comparable<Task> to support sorting Update README.md to fix formatting errors.
Merge branch-C-Sort onto master
Reconfigure GUI to use FXML Refine some elements of GUI
Merge branch-A-BetterGui onto master
Modify some responses to commands Fix lack of welcome message
Merge branch-A-Personality onto master
Handle more anticipated errors including index out of bounds, invalid index
Merge branch-A-MoreErrorHandling onto master
Update README.md with additional information to serve as a User Guide Add Ui.png, representative screenshot of Book Remove text-ui-test/ (obsolete)
Update README.md, docs/README.md to support setup of product page
Minor tweaks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Book
Book lets you write stuff in it. It's,
To get a Book of your own, simply,
.jarfile.Features:
No one's stopping you from jumping right into the code either, here's the
mainmethod: