[Kea Harvan Suyanto] iP #459
Open
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
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.
Add save feature from branch-level-7
Add minimal date and time functionality from branch-level-8
Add storage class
Add gradle support
add j-unit testcases
Follow coding standard
Add JavaDoc
Add automated UI testing
src/main/java/shep/Shep.class
Outdated
There was a problem hiding this comment.
You should add .class files to your gitignore, otw there is uneccessary bloat.
There was a problem hiding this comment.
As compared to 1 Commands class, we can try to extract the functionality out into multiple classes.
There was a problem hiding this comment.
when parsing the date the code should be put in a try catch block to make sure we handle incorrect input
src/main/java/shep/task/Event.java
Outdated
bkkuan
reviewed
Feb 6, 2025
src/main/java/Interaction.java
Outdated
| import java.io.IOException; | ||
| import java.nio.file.Files; | ||
| import java.nio.file.Path; | ||
| import java.nio.file.Paths; | ||
|
|
||
| public class Interaction { | ||
| TaskList list; |
There was a problem hiding this comment.
Fields should generally be set to private for encapsulation
src/main/java/Interaction.java
Outdated
|
|
||
| public Interaction() { | ||
| this.list = new TaskList(); | ||
|
|
||
| // load or initialise data directory | ||
| Path dataDirectoryPath = Paths.get("../data"); |
There was a problem hiding this comment.
Its better to define constants for better maintainability
eg. private static final String DATA_DIRECTORY = "../data";
Then , Path dataDirectoryPath = Paths.get(DATA_DIRECTORY);
src/main/java/Interaction.java
Outdated
| Scanner userInputScanner = new Scanner(System.in); | ||
| boolean saidBye = false; | ||
| while (!saidBye) { | ||
| String currUserInputText = userInputScanner.nextLine(); | ||
| saidBye = Commands.executeCommand(currUserInputText, this.list, this.filePath, true); |
There was a problem hiding this comment.
executeCommand() is called twice which may cause unintended behaviour.
src/main/java/TaskList.java
Outdated
| import java.util.Iterator; | ||
| import java.io.FileWriter; | ||
| import java.io.IOException; | ||
| import java.nio.file.Files; | ||
| import java.nio.file.Path; | ||
| import java.util.ArrayList; |
There was a problem hiding this comment.
java.util.ArrayList is imported twice. Remove the duplicate
src/main/java/TaskList.java
Outdated
|
|
||
| public void writeToFile(Path filePath) { | ||
| // overwrite previous taskList to be empty, no need to add anything to the file | ||
| try (FileWriter fileWriter = new FileWriter(filePath.toString(), false)) { |
There was a problem hiding this comment.
this block is redundant
FileWriter is opened but not used, which is pointless
src/main/java/TaskList.java
Outdated
| import java.util.Iterator; | ||
| import java.io.FileWriter; | ||
| import java.io.IOException; | ||
| import java.nio.file.Files; | ||
| import java.nio.file.Path; | ||
| import java.util.ArrayList; | ||
|
|
||
| public class TaskList extends ArrayList<Task> { |
There was a problem hiding this comment.
generally a bad practice as it violates encapsulation
Some code is not of quality. Mostly, SLAP. Shep.java is also not needed anymore after moving over to GUI. Refactor the code to improve code quality and remove Shep.java. Code was refactored in order to adhere to SLAP. Several classes had multiple layers of abstraction so method extraction had to be done.
Some methods depended on the logic of other functions. To ensure that the logic is followed so that it can be utilised correctly by the class Assertations were added. Add Assertations to methods that require other methods to follow their logic properly. Apart fromthe Commands class assertations never replaced if else statements. This is because assertations shouldn't be used to control the program flow.
Logic bug regarding asserts replacing if-else statements caused program to not be able to add tasks. Reverted assert statements back to if-else. If else statements worked before hand.
Refactor code, remove Shep.java
Fix merge conflict Several merge conflicts when trying to merge master with assertions added. Cleaned up. Should be.
Add assertations to reinforce logic
Add CI functionality using gradle.yml file under dir ./github/workflows
On the addition of any type of Task into a TaskList, no duplicate checks are made, allowing the chance for a duplicate task to be added into the test list. This is a redundant feature and duplicate Tasks shouldn't be allowed. In order to fix this, a checkDuplicates was added in the TaskList class as well as an equals method in Task. Also updated TaskListTests and CommandsTest classes to account for newer situations.
On the addition of any type of Task into a TaskList, no duplicate checks are made, allowing the chance for a duplicate task to be added into the test list. This is a redundant feature and duplicate Tasks shouldn't be allowed. In order to fix this, a checkDuplicates was added in the TaskList class as well as an equals method in Task. Also updated TaskListTests and CommandsTest classes to account for newer situations.
Refactor Commands.java
Add User Guide
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.
No description provided.