Skip to content

[Shen Jiaming] iP#105

Open
Oshen27 wants to merge 45 commits into
nus-cs2113-AY2425S1:masterfrom
Oshen27:master
Open

[Shen Jiaming] iP#105
Oshen27 wants to merge 45 commits into
nus-cs2113-AY2425S1:masterfrom
Oshen27:master

Conversation

@Oshen27

@Oshen27 Oshen27 commented Sep 3, 2024

Copy link
Copy Markdown

No description provided.

damithc and others added 8 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.
Comment thread src/main/java/Bron.java Outdated
Comment on lines +5 to +10
String logo = " ____ ____ ____ _ _\n"
+ " | _ \\ | _\\ / __ \\ | \\ | |\n"
+ " | |_) | | |_) | | | | | | \\ | | \n"
+ " | _ < | ___/ | | | | | |\\\\ | |\n"
+ " | |_) | | | \\ \\ | |__| | | | \\\\| |\n"
+ " |____/ |_| \\_\\ \\____/ |_| \\__|\n";

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Should this be a static variable?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Should logo be capitalised as LOGO instead to follow the naming convention for constants?

Comment thread src/main/java/Bron.java Outdated
+ " | |_) | | | \\ \\ | |__| | | | \\\\| |\n"
+ " |____/ |_| \\_\\ \\____/ |_| \\__|\n";
Scanner input = new Scanner(System.in);
System.out.println(logo + "Hello! I'm Bron\n" + "What can I do for you?\n");

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You may change your name into a variable for easier reuse & modification (if any)

Comment thread src/main/java/Bron.java Outdated
Comment on lines +20 to +51
if (line.equalsIgnoreCase("bye")) {
System.out.println("Catch you on the flip cuh");
break;
}

if (line.equalsIgnoreCase("list")) {
int listCount = 0;
while(tasks[listCount] != null) {
System.out.println(listCount + 1 + ". " + tasks[listCount++].printTask());
}
} else if (line.startsWith("mark")){
int taskIndex = Integer.parseInt(line.split(" ")[1]) - 1;
if (taskIndex >= 0 && taskIndex < taskCount) {
tasks[taskIndex].markAsDone();
System.out.println("Good shit kid! I've marked this task as done:");
System.out.println(" " + tasks[taskIndex].printTask());
} else {
System.out.println("Task not found.");
}
} else if (line.startsWith("unmark")) {
int taskIndex = Integer.parseInt(line.split(" ")[1]) - 1;
if (taskIndex >= 0 && taskIndex < taskCount) {
tasks[taskIndex].markAsNotDone();
System.out.println("Get yo shit together son, this task aint done yet:");
System.out.println(" " + tasks[taskIndex].printTask());
} else {
System.out.println("Task not found.");
}
} else {
System.out.println("added" + ": " + line);
tasks[taskCount++] = new Task(line);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You may separate the process of each command into methods for readability and reusability.

Should the command words (ex: "mark", "list", ...) be static variables?

Comment thread src/main/java/Task.java Outdated
Comment on lines +22 to +24
public String printTask() {
return "[" + getStatusIcon() + "] " + this.description;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

As the method is returning a String, not printing it out, should its name be changed to getTaskString() or something like that?

Comment thread src/main/java/Bron.java Outdated
Comment on lines +5 to +10
String logo = " ____ ____ ____ _ _\n"
+ " | _ \\ | _\\ / __ \\ | \\ | |\n"
+ " | |_) | | |_) | | | | | | \\ | | \n"
+ " | _ < | ___/ | | | | | |\\\\ | |\n"
+ " | |_) | | | \\ \\ | |__| | | | \\\\| |\n"
+ " |____/ |_| \\_\\ \\____/ |_| \\__|\n";

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Should logo be capitalised as LOGO instead to follow the naming convention for constants?

Comment thread src/main/java/Bron.java Outdated
Task[] tasks = new Task[100];
int taskCount = 0;

while (true) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Should the main function be shortened (eg. with more abstraction)? This main function is quite hard to read for me as it is quite long.

Comment thread text-ui-test/runtest.sh

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Should there be something in this file to prevent people from being mislead, or at least, a TODO in this file to understand what needs to be done?

Comment thread src/main/java/Bron.java Outdated
while(tasks[listCount] != null) {
System.out.println(listCount + 1 + ". " + tasks[listCount++].printTask());
}
} else if (line.startsWith("mark")){

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This looks like arrowhead code, and I feel with more abstraction we could reduce this effect

Created Command enum, CommandHandler class and ChatBotManager class to improve readability and organization of code
Added new Exception classes and updated error handling mechanism in CommandHandler
Added new Exception classes and updated error handling mechanism in CommandHandler
Comment thread src/main/java/bron/Bron.java Outdated
}

private static Task[] initializeTasks() {
return new Task[100];

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Avoid the use of magic numbers.

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.

6 participants