Skip to content

[Zhou Qi] iP#64

Open
cczhouqi wants to merge 30 commits into
nus-cs2113-AY2122S2:masterfrom
cczhouqi:master
Open

[Zhou Qi] iP#64
cczhouqi wants to merge 30 commits into
nus-cs2113-AY2122S2:masterfrom
cczhouqi:master

Conversation

@cczhouqi

@cczhouqi cczhouqi commented Feb 1, 2022

Copy link
Copy Markdown

No description provided.

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

There's a few suggestions I made for some areas of the code. Other than that, you might want to look at things to implement to give your own bot more personality so that it can stand out from the given template.

Comment thread src/main/java/Duke.java Outdated
import java.util.Scanner;

public class Duke {
public static String boundary = "____________________________________________________________" + System.lineSeparator();

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 how you have included this boundary for ease of printing. Nice work!

Comment thread src/main/java/Duke.java Outdated
Comment on lines +28 to +39
public static void addTask(String request) {
if (request.toLowerCase().startsWith("deadline")) {
int byPosition = request.indexOf("/");
taskList[countTask] = new Deadline(request.substring(9, byPosition - 1), request.substring(byPosition + 4));
} else if (request.toLowerCase().startsWith("event")) {
int atPosition = request.indexOf("/");
taskList[countTask] = new Event(request.substring(6, atPosition - 1), request.substring(atPosition + 4));
} else if (request.toLowerCase().startsWith("todo")) {
taskList[countTask] = new Todo(request.substring(5));
} else {
taskList[countTask] = new Task(request);
}

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 can use a switch statement instead of multiple if-else statements? I noticed the same issue in your main as well

Comment thread src/main/java/Duke.java Outdated
line = in.nextLine();
}
System.out.print(boundary + "Bye. Hope to see you again soon!" + System.lineSeparator() + boundary);
sayGoodbye();

@jltha jltha Feb 3, 2022

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 how you've abstracted the printing of your exit message. But perhaps the same can also be done for your welcome message and execution of inputs? You getting close, dont give up!

Comment thread src/main/java/Event.java Outdated
Comment on lines +10 to +14
@Override
public String toString() {
return "[E]" + super.toString() + " (at: " + at + ")";
}
}

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 see your usages of polymorphism in all your classes. Keep up the good work!

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

Looks great overall! The variable and method names are named appropriately and apart from the minor cosmetic changes I suggested, the code is easy to read.

But I would also suggest for you to edit the text-ui-test files. It would not only help in debugging, but other reviewers can also see the output of your code without having to run it on their own IDEs.

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

public static void printList() {
System.out.println(boundary + "Here are the tasks in your list:");
for (int i = 0; i < countTask; 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.

In the for loop, you could remove the space in "i ++", such that it follows this suggested coding guideline in the se-edu website:
image

Comment thread src/main/java/Duke.java Outdated
taskList[countTask] = new Task(request);
}

countTask ++;

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 spacing between "countTask" and "++" could be removed too.

Comment thread src/main/java/Duke.java Outdated
+ "| |_| | |_| | < __/\n"
+ "|____/ \\__,_|_|\\_\\___|\n";
System.out.println("Hello from\n" + logo);
System.out.println(boundary+ logo);

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 was probably a typo error, but you could add a space between "boundary" and "+"

Comment thread src/main/java/Duke.java Outdated
System.out.println("What can I do for you?" + System.lineSeparator() + boundary);
Scanner in = new Scanner(System.in);
String line = in.nextLine();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggestion: Maybe this extra blank line can be removed? It might make the formatting look more standardised.

Comment thread src/main/java/Event.java Outdated
@@ -0,0 +1,14 @@
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.

I think this blank line can be removed? (So that it follows the formatting of the rest of your code)

Comment thread src/main/java/Deadline.java Outdated
@@ -0,0 +1,14 @@
public class Deadline 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.

I think this blank line can be removed? (So that it follows the formatting of the rest of your code, where the class's attributes are directly under the class call)

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