-
Notifications
You must be signed in to change notification settings - Fork 429
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[hyizhak] iP #448
base: master
Are you sure you want to change the base?
[hyizhak] iP #448
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.
Merge Level-7 Save Branch to the master
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Just missing the packages for the classes as well as the javadocs. Code looks clean.
src/main/java/Bard.java
Outdated
public static void main(String[] args){ | ||
Bard bard = new Bard(); | ||
bard.run(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No JavaDocs written for the methods and the class
src/main/java/BardException.java
Outdated
@@ -0,0 +1,10 @@ | |||
public class BardException extends Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly, no JavaDocs written for this class.
src/main/java/Bard.java
Outdated
@@ -0,0 +1,164 @@ | |||
import java.time.LocalDate; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing package for the class.
https://se-education.org/guides/conventions/java/intermediate.html#:~:text=Put%20every%20class%20in%20a%20package
src/main/java/BardException.java
Outdated
@@ -0,0 +1,10 @@ | |||
public class BardException extends Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Class is not inside a package.
https://se-education.org/guides/conventions/java/intermediate.html#:~:text=Put%20every%20class%20in%20a%20package
src/main/java/BardStorage.java
Outdated
@@ -0,0 +1,83 @@ | |||
import java.io.File; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Class is not inside a package.
https://se-education.org/guides/conventions/java/intermediate.html#:~:text=Put%20every%20class%20in%20a%20package
src/main/java/Event.java
Outdated
public String toFileString() { | ||
return "E | " + super.toFileString() + " | " + from + "-" + to; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing JavaDocs for the class and its methods.
src/main/java/Task.java
Outdated
@@ -0,0 +1,31 @@ | |||
public class Task { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Class is not inside a package.
https://se-education.org/guides/conventions/java/intermediate.html#:~:text=Put%20every%20class%20in%20a%20package
src/main/java/Task.java
Outdated
|
||
public Task(String description) { | ||
this.description = description; | ||
this.isDone = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing JavaDocs for the class and its methods.
src/main/java/Todo.java
Outdated
@@ -0,0 +1,18 @@ | |||
public class Todo extends Task { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Class is not inside a package.
https://se-education.org/guides/conventions/java/intermediate.html#:~:text=Put%20every%20class%20in%20a%20package
src/main/java/Todo.java
Outdated
public String toFileString() { | ||
return "T | " + super.toFileString(); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing JavaDocs for the class and its methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, in terms of coding syntax, it is mostly compliant. The only missing things are the javaDoc and the packaging of the classes. Code is easily understandable, although packaging it into packages would make it easier to understand how the various function interact.
src/main/java/DateParser.java
Outdated
case "fri", "friday" -> DayOfWeek.FRIDAY; | ||
case "sat", "saturday" -> DayOfWeek.SATURDAY; | ||
case "sun", "sunday" -> DayOfWeek.SUNDAY; | ||
default -> throw new IllegalArgumentException("Invalid day: " + day); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting use of switch case to directly return the right enum variable . The formatting looks correct
src/main/java/Bard.java
Outdated
tasks.remove(taskNumber - 1); | ||
System.out.println(" Now you have " + tasks.size() | ||
+ " tasks in the list." + "\n" + horizontalLine); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be good to package all functions above in a command package/parser package
…function from Text UI - Added assertions in the command parser to validate critical assumptions before executing commands. - Integrated assertions in the main program to ensure that key invariants are maintained. - Implemented the exit function from the Text UI, allowing for a controlled shutdown of the application. These changes enhance the overall robustness and reliability of the application, ensuring that failures in critical areas are caught early and the exit behavior is clearly defined.
- Refactored the command parser's createTask method: - Replaced the if-else chain with a switch statement for better clarity. - Introduced guard clauses and input trimming to validate command inputs early. - Improved error handling by throwing descriptive exceptions for missing or malformed arguments. - Refactored the DateParser class: - Consolidated time parsing logic to avoid redundancy and improve maintainability. - Enhanced input validation and error messaging for both date and day formats. - Added a private constructor to enforce the utility class pattern. These improvements result in more robust, readable, and maintainable parsing logic across the application.
- Replaced switch statement in CommandParser.parse with enhanced switch
feat: Add assertions in command parser and main program; set up exit function from Text UI
- Merged latest changes from master into the PR branch. - Manually resolved conflicts in src/main/java/bard/parser/CommandParser.java by integrating modifications from both branches. - Ensured that command parsing logic remains consistent and all tests pass post-resolution. - Cleaned up redundant code and aligned with upstream changes to maintain feature integrity.
Code quality enhancement
- Added a new workflow file ("Java CI") to automatically build and test the project on push and pull_request events. - Configured a build matrix to run on ubuntu-latest, macos-latest, and windows-latest for broad platform coverage. - Included steps to: - Check out the repository (with a specific checkout for the master branch). - Merge the current commit using a forceful checkout. - Validate the Gradle Wrapper via the gradle/wrapper-validation-action. - Set up JDK 17 with JavaFX support using actions/setup-java. - Build the project and run tests using './gradlew check'. - Execute IO redirection tests on Linux, macOS, and Windows with respective scripts. These changes ensure consistent, automated testing and build verification across multiple operating systems.
CI: set up cross-platform Java CI pipeline with GitHub Actions
- Added comprehensive Javadoc comments to public classes and methods across the project (e.g., TaskList, CommandParser, DateParser, etc.). - Reorganized import orders and adjusted whitespace to address Checkstyle violations. - Ran Spotless to auto-apply formatting fixes, ensuring that the code conforms to our style rules. These changes enhance code readability, documentation quality, and maintain consistent style throughout the codebase.
Add required JavaDoc so the code can pass CI
- Implemented a new compareTo method for tasks: - Incomplete tasks are prioritized over completed ones. - Deadline tasks are sorted based on their 'by' time. - Event tasks are sorted based on their 'from' time. - Ensured tasks marked as done always appear at the bottom. - Integrated the updated sorting logic into the chatbot functionality to enhance task management. This update improves task prioritization and overall user experience within the chatbot.
feat: Add task sorting and integrate into chatbot functionality
Add more JUnit testing for Task class
Modify the styling css to highlight errors
Adjust the logic of storage-related exception
Add the user guide in readme.md
Bard (iP) Update
Bard (iP) frees your mind from the burden of memorization. It's:
FastSUPER FAST to useHere are some steps to get started:
java -jar "bard.jar"
Features
If you are a Java programmer, you can use Bard to practice Java too. Here’s the
main
method for reference: