Skip to content
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

Wang Haitao iP #449

Open
wants to merge 20 commits into
base: master
Choose a base branch
from
Open

Conversation

HAITAO2003
Copy link

No description provided.

damithc and others added 17 commits July 11, 2024 16:52
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.
1. Better modularisation of the code
2. added delete function
3. fixed output format problem
Copy link

@DESU-CLUB DESU-CLUB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All in all well done! There were a few violations but it was a really good job with the overall naming conventions

Jiayouzz for IP!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might want to git ignore the .class

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great naming conventions here for the formats!

} else {
try {
switch(parts[0]) {
case "deadline":

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the coding standards, cases have no indentations (same indent as switch)

@@ -0,0 +1,68 @@
import utils.Utils;

import java.util.ArrayList;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be better to put ArrayList before custom imports like utils, as that caused me to violate the checkstyle

However the coding standards dont enforce this to that degree so this is really up to your personal preference

@@ -3,7 +3,7 @@
import java.util.Optional;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like how you're using typing in order to annotate your classes and allow for empties!

Comment on lines 15 to 45
public void add_item(Task item) {
this.list.add(item);
System.out.println("Got it. I've added this task:");
printTask(item);
Utils.output("Now you have " + this.list.size() + " tasks in the list.");
saveToFile();
}

public void delete_item_by_index(int index) throws DukeException {
if (index < 1 || index > list.size()) {
throw new DukeException("Invalid task number! Please provide a number between 1 and " + list.size());
}
Task removedTask = list.remove(index - 1);
System.out.println("Noted. I've removed this task:");
printTask(removedTask);
Utils.output("Now you have " + this.list.size() + " tasks in the list.");
saveToFile();

}

public void mark_done_by_index(int idx) {
if (idx > this.list.size()){
Utils.output("Task number does not exist!");
}else{
System.out.println("Nice! I've marked this task as done:");
Task item = this.list.get(idx - 1);
item.markDone();
printTask(item);
saveToFile();
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please follow the naming convention for class methods.
Suggested:
addItem(args)
deleteItem(args)
markDone(args)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants