[shadhankkk] iP #649
Open
[shadhankkk] iP #649
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.
weiseng18
reviewed
Sep 6, 2024
src/main/java/duke/Duke.java
Outdated
| /** | ||
| * Returns void, just prints a horizontal line | ||
| */ | ||
| void printHorizontalLine() { |
There was a problem hiding this comment.
method names follow convention (start with verb, written in camelCase) and are really organized! great work on that
weiseng18
reviewed
Sep 6, 2024
src/main/java/duke/Duke.java
Outdated
| int len = userInput.length(); | ||
| int startIndex = ptr; | ||
|
|
||
| while(ptr < len) { |
There was a problem hiding this comment.
The recommended Java coding standard requires a space after reserved keywords such as while. But you'll probably figure this out after adding checkstyle in Week 4, so you can fix it then!
weiseng18
reviewed
Sep 6, 2024
src/main/java/duke/Duke.java
Outdated
|
|
||
| ui.printHorizontalLine(); | ||
|
|
||
| for(int i=0; i<numberOfMessages; i++) { |
weiseng18
reviewed
Sep 6, 2024
src/main/java/duke/Duke.java
Outdated
| * in our array of tasks | ||
| */ | ||
| void markTask(int rank) { | ||
| if(rank < 1 || rank > tasks.size()) { |
weiseng18
reviewed
Sep 6, 2024
There was a problem hiding this comment.
overall very organized code!
there are some minor coding style issues that you'll probably notice after adding checkstyle in Week 4, so you can fix it once you've worked on that!
you might want to add .class and .jar to .gitignore since we don't want to commit binaries
yooplo
reviewed
Sep 6, 2024
src/main/java/duke/Duke.java
Outdated
| /** | ||
| * Returns void, just prints the opening text | ||
| */ | ||
| void printOpening() { |
There was a problem hiding this comment.
Looks good!I don't know if adding access modifiers to the methods would make it better though.
sandyk0105
reviewed
Sep 6, 2024
src/main/java/duke/Duke.java
Outdated
| * Returns void, just prints the latest task unmarked | ||
| * | ||
| * @param task the task that was unmarked | ||
| */ | ||
| void printTaskUnmarked(Task task) { | ||
| System.out.println(horizontalLine); | ||
| System.out.println("OK, I've marked this task as not done yet:"); | ||
| task.print(); | ||
| System.out.println(horizontalLine); | ||
| } | ||
|
|
There was a problem hiding this comment.
Ui class has all needed methods, using abstraction very very well.
Some parts of the code assume certain conditions hold, but these assumptions are not explicitly enforced. This could lead to errors or unexpected behavior during execution if those assumptions are violated. Without enforcing these conditions, the code may fail silently, which could make debugging more difficult and reduce code reliability. Let's add assert statements to verify critical conditions at runtime. This ensures that assumptions are checked and provides immediate feedback if they are violated. Using assert is appropriate here because it adds minimal overhead and is disabled in production, keeping performance unaffected.
The program contains blocks of code that are no longer executed or necessary. These portions of the codebase add unnecessary complexity and make it harder to maintain and read the code. Dead code can confuse developers and lead to misunderstandings about the program's behavior. It's important to keep the codebase clean and focused only on what is required. Let's remove the dead code to simplify the codebase, reduce maintenance overhead, and improve overall readability. Removing dead code helps avoid future bugs and keeps the project lean.
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.