Skip to content

[paklongchiu] iP#192

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

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

Conversation

@paklongchiu

Copy link
Copy Markdown

No description provided.

@Ashertan256 Ashertan256 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.

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!

Comment thread src/main/java/Elyk.java
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"));

Comment thread src/main/java/Elyk.java Outdated
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?

Comment thread src/main/java/Elyk.java Outdated

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

Comment thread src/main/java/Elyk.java Outdated
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

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

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

@thomasjlalba thomasjlalba 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 thus far! Just some trivial suggestions to make it more readable and understandable for others.

Comment thread src/main/java/Elyk.java Outdated
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

Comment thread src/main/java/Elyk.java Outdated
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

Comment thread src/main/java/Elyk.java Outdated
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)

Comment thread src/main/java/Elyk.java Outdated
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 thread src/main/java/Elyk.java Outdated
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.

Comment thread src/main/java/Elyk.java Outdated
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