-
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
[Xzh119] iP #450
base: master
Are you sure you want to change the base?
[Xzh119] iP #450
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.
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 I found your code easy to read for the most part, except for some areas where there were minor coding standard errors such as missing spacing and lines. You might want to consider splitting some single lines into several lines so it is easier to read.
src/main/java/Samantha.java
Outdated
} else { | ||
if (taskSum > 99) { | ||
System.out.println(" List is full."); | ||
}else { |
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.
Should there be a space between { and else?
src/main/java/Task.java
Outdated
this.isDone = false; | ||
} | ||
|
||
public String getStatusIcon() { |
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.
Good use of a camelCase for method naming.
src/main/java/Task.java
Outdated
@@ -0,0 +1,26 @@ | |||
public class Task { | |||
protected String description; | |||
protected boolean isDone; |
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.
Good use of a boolean naming convention.
src/main/java/SamanthaException.java
Outdated
public class SamanthaException extends Exception { | ||
public SamanthaException(String message) { | ||
super(message); | ||
} | ||
} |
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.
Should there be more specific exceptions e.g. NumberFormatException, IndexOutOfBoundsException ?
src/main/java/Samantha.java
Outdated
@@ -0,0 +1,140 @@ | |||
import java.util.Scanner; | |||
import java.util.ArrayList; |
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.
You might want to leave a line below this statement before defining your public class
# Conflicts: # src/main/java/Task.java
* 'master' of https://github.com/Xzh119/ip: Test b Test a
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.
The code LGTM, despite some possible improvement in following coding standard. Additaionally, I like how the naming explain the usage of a field/method clearly.
src/main/java/Task.java
Outdated
String type = parts[0]; | ||
|
||
switch (type) { | ||
case "T": |
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.
Maybe the indentation for case
clauses could be removed according to coding standard?
src/main/java/Task.java
Outdated
@@ -0,0 +1,48 @@ | |||
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.
Should Task
be an abstract class instead?
src/main/java/Deadline.java
Outdated
String[] parts = line.split(" \\| "); | ||
boolean isDone = parts[1].equals("1"); | ||
Deadline task = new Deadline(parts[2], parts[3]); | ||
if (isDone) task.markAsDone(); |
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.
Maybe if
should be followed by a block?
The `parseCommand` method does not have assertions to verify the validity of inputs. This may lead to unexpected runtime errors. Add assertions to ensure inputs meet expected conditions. Assertions check for null command and properly formatted commands. This helps catch potential bugs early and improves code robustness by preventing invalid operations before they can cause failures.
Merge Branch-a-assertions
Improve Code Quality
Revert "Improve Code Quality"
Merge branch 'master' into branch-A-CodeQuality
It needs to be change as it affects the purpose of listing. It is fixed by preventing the "\n" at the end. It is done this way because it is reasonable and easy.
Samantha
Samantha, named after the emotional AI in the movie HER, frees your mind of having to remember things you need to do. It's,
FASTSUPER FAST to useAll you need to do is,
And it is FREE!
Features:
If you are a Java programmer, you can use it to practice Java too. Here's the main method: