Skip to content

[paklongchiu] iP#192

Open
paklongchiu wants to merge 31 commits intonus-cs2113-AY2425S2:masterfrom
paklongchiu:master
Open

[paklongchiu] iP#192
paklongchiu wants to merge 31 commits intonus-cs2113-AY2425S2:masterfrom
paklongchiu:master

Conversation

@paklongchiu
Copy link
Copy Markdown

No description provided.

Copy link
Copy Markdown

@Ashertan256 Ashertan256 left a comment

Choose a reason for hiding this comment

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

Overall, the structure and flow of the code seems a little redundant at certain parts. But the format looks to be consistent with the course!

public static void main(String[] args) {
greet();

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.

Instead of while(true), maybe you can consider using this instead:

do {
// code to run
}
while (!input.equal("bye"));

public static void markTaskNotDone(int taskNum) {
taskList[taskNum - 1].markAsNotDone();
System.out.println("OK, I've marked this task as not done yet:");
System.out.println(" [ ] " + taskList[taskNum - 1].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.

Would using taskList[i].getStatusIcon (as you used below) be more consistent?


public static void updateInput() {
input = command.nextLine();
if (input.matches(".*\\d$")) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

A comment could be added to describe the regex pattern to be clearer

if (input.matches(".*\\d$")) {
String[] words = input.split(" ");
input = words[0];
taskNum = Integer.parseInt(words[1]);
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 could potentially have issues if the second word is not a number. Attempting to parseInt a String may crash the program/cause an exception

}

public static void markTaskNotDone(int taskNum) {
taskList[taskNum - 1].markAsNotDone();
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 seems like a redundant method, is it possible to call a single method without chaining multiple methods that do the same thing?

Thomas Chiu added 2 commits February 14, 2025 12:28
like input task type is not supported or there is missing information from the input
Copy link
Copy Markdown

@thomasjlalba thomasjlalba left a comment

Choose a reason for hiding this comment

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

Good job thus far! Just some trivial suggestions to make it more readable and understandable for others.

Comment on lines +77 to +79
to = input.substring(toPos + 4);
from = input.substring(fromPos + 6, toPos - 1);
description = input.substring(6, fromPos - 1);
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 you can refactor the magic literals throughout your code to make it easier to understand

from = input.substring(fromPos + 6, toPos - 1);
description = input.substring(6, fromPos - 1);
input = "event";
}
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 you can include the final else statement for error detection

public static void printTask() {
System.out.println("Here are the tasks in your list:");
for (int i = 0; i < taskCounter; i++) {
System.out.println((i+1) + "." + taskList[i]);
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 you can standardise the use of spacing within your i+1 to make it more consistent (i + 1 instead)

markTaskNotDone(taskNum);
break;
case "todo":
taskList[taskCounter++] = new Todo(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.

It might be more readable by separating the increment to the next line instead of combining it in the same line

Comment on lines +4 to +5
public static int taskCounter = 0;
public static int taskNum = 0;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The naming of these variables might suggest that they are the same. Maybe you can consider renaming them to make them clearer and more distinct.

public static Scanner command = new Scanner(System.in);

public static void main(String[] args) {
greet();
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 on the abstraction of code to make it neater!

Thomas Chiu and others added 19 commits February 21, 2025 12:33
… created Ui and Parser class

Modified code in Elyk and Storage to use TaskList instead
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.

3 participants