Skip to content

[Chua Zhong Heng] iP#63

Open
Cazh1 wants to merge 53 commits into
nus-cs2113-AY2324S1:masterfrom
Cazh1:master
Open

[Chua Zhong Heng] iP#63
Cazh1 wants to merge 53 commits into
nus-cs2113-AY2324S1:masterfrom
Cazh1:master

Conversation

@Cazh1

@Cazh1 Cazh1 commented Sep 4, 2023

Copy link
Copy Markdown

No description provided.

@karishma-t karishma-t 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! good use of encapsulation

Comment thread src/main/java/Nuke.java Outdated
public static void printLine() {
System.out.println(" ____________________________________________________________");
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Can consider adding javadoc comments for each method for greater clarity.

Comment thread src/main/java/Nuke.java Outdated
add(itemList, markList, item);
}
dialogue(itemList, markList);
}

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 use of naming standards, e.g camelCase

Comment thread src/main/java/Nuke.java Outdated
}
printLine();
}

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 layout structure

Comment thread src/main/java/Nuke.java Outdated
int listIndex = Integer.parseInt(splitItem[1]);
unmark(itemList, markList, listIndex);
} else {
add(itemList, markList, item);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

indentations just require one more space

@ShaniceTang ShaniceTang 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, your coding quality is very well done and your code is readable! Great job! 💯

Comment thread src/main/java/Nuke.java Outdated
printLine();
System.out.println(item);
printLine();
echo();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nice!
I like that you use recursion :)

Comment thread src/main/java/Nuke.java Outdated
int listIndex = Integer.parseInt(splitItem[1]);
mark(itemList, markList, listIndex);
} else if (item.length() > 6 && item.substring(0, 6).equals("unmark")) {
String[] splitItem = item.split(" ");

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Could this array be named "items" instead?

Comment thread src/main/java/Nuke.java Outdated
printLine();
}

public static void main(String[] args) {

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 your main method is short and neat :D

Comment thread src/main/java/Nuke.java Outdated
public static void dialogue(ArrayList<String> itemList, ArrayList<String> markList) {
Scanner input = new Scanner(System.in);
String item = input.nextLine();
if (item.equals("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.

I like that you use else-if instead of nesting 👍

@woowenjun99 woowenjun99 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 on your first milestone! Not to bad, just that perhaps you might want to consider replacing all the magic number and magic string with variables and not sure if I am correct, there is a recursive call.

Comment thread src/main/java/Event.java Outdated
@@ -0,0 +1,15 @@
public class Event extends Task{

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Perhaps you want to add an additional space in front of the curly paranthesis?

Comment thread src/main/java/KenergeticBot.java Outdated
printLine();
System.out.println(" Here are the tasks in your list:");
for (int i = 0; i < taskList.size(); i++) {
System.out.printf(" %d.%s\n", i+1, taskList.get(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.

Nice to use printf!

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

public static void printGreetingMessage() {
printLine();
System.out.println(" Hello! I'm KenergeticBot\n" + " What can I do for you?");

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Perhaps you would like to replace all the magic strings and numbers in your code with variables for greater readability?

Comment thread src/main/java/KenergeticBot.java Outdated
System.out.println(KenergeticBotException.INVALID_COMMAND); // unable to print sad face ˙◠˙
}
}
botDialogue(taskList);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Is this recursion???

Comment on lines +4 to +5


Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Perhaps you would like to remove these extra spaces?

Comment thread src/main/java/Task.java Outdated
Comment on lines +4 to +5
protected String taskType;
protected int taskIndex;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Perhaps you would like to remove this unused variable?

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.

4 participants