Skip to content

[Tyrus Lye]iP#557

Open
TyrusLye wants to merge 36 commits into
nus-cs2103-AY2324S1:masterfrom
TyrusLye:master
Open

[Tyrus Lye]iP#557
TyrusLye wants to merge 36 commits into
nus-cs2103-AY2324S1:masterfrom
TyrusLye:master

Conversation

@TyrusLye

@TyrusLye TyrusLye commented Sep 4, 2023

Copy link
Copy Markdown

No description provided.

@remuslum remuslum left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Good job, keep it up 💯

@aarontxz aarontxz left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code seems logical and can handle tasks given, could package more chunks of codes into functions to make the code neater. Some quality of life changes for the user are commented too.

Comment thread src/main/java/Duke.java Outdated
String command = scanner.nextLine();
System.out.println("____________________________________________________________");
try{
if (command.equalsIgnoreCase("bye")) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

would it be better to use enumerates for the commands?

Comment thread src/main/java/Duke.java Outdated
}
} catch (FileNotFoundException e) {
// Handle file not found exception
System.out.println("File not found: " + FILE_PATH);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I like that the handling of this exception not only tells that user that the file cannot be found but also returns the filepath

Comment thread src/main/java/Duke.java Outdated
for (int i = 0; i < tasks.size(); i++) {
System.out.println((i + 1) + ". " + tasks.get(i));
}
} else if (command.startsWith("todo")) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

would it be better to package some of these if, else if else steps into a function that handles everything within the outer most if case? for eg. a function to handle creating todo here.

Comment thread src/main/java/Duke.java Outdated
tasks.get(index).markDone();
System.out.println("Nice! I've marked this task as done:\n " + tasks.get(index));
} else {
System.out.println("Invalid task index.");

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

would it be useful if instead of printing invalid task index, the size of the tasklist is also returned in this else case to let user know that for example there is only 3 tasks in the tasklist thats why their input 4 is invalid. Something like "Invalid task index, the task list has " + tasklist.size() + "tasks. "

@nananakx-x nananakx-x left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM overall!! The naming of the methods are all of the course standard, just that it might be better for you to start adding javadoc to your public methods and also take note of the indentation of switch statements. Readability can also be improved if commands are split into different methods to handle each case (instead of putting all in one method). Good job so far! :D

Comment thread src/main/java/Duke.java Outdated
if (command.equalsIgnoreCase("bye")) {
System.out.println("Bye. Hope to see you again soon!");
break;
} else if (command.equalsIgnoreCase("list")) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Maybe it will be better if this part is separated into different functions like addToDoTask, etc. to improve readability, what do you think?

Comment thread src/main/java/Duke.java Outdated
String description = parts[2];

Task task = null;
switch (type) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

For switch statements, I think there shouldn't be indentation on the next line. On the mod website:
switch (condition) {
case ABC:
statements;
// Fallthrough
case DEF:
statements;
break;
case XYZ:
statements;
break;
default:
statements;
break;
}

Comment thread src/main/java/Duke.java Outdated
}

@Override
public String toString() {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Clean and concise, good job!

Comment thread src/main/java/Duke.java Outdated
Scanner scanner = new Scanner(System.in);
ArrayList<Task> tasks = new ArrayList<>(100);
loadTasksFromFile(tasks);
String logo = "██╗░░░██╗██████╗░██████╗░░█████╗░██╗\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.

This is cool!!

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