Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Lim Wen Feng] iP #98

Open
wants to merge 65 commits into
base: master
Choose a base branch
from
Open

Conversation

limwenfeng
Copy link

No description provided.

Finish implementing level 1
Add, List
Mark as Done
Follow coding standard
Upgraded to level 4
Change to follow convention coding standard
tasksCount++;
System.out.println("Added: " + userCommand[0]);
notifyUser(inputDetails, selectedTask);

Choose a reason for hiding this comment

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

can remove the blank line

Copy link

@rageqqq rageqqq left a comment

Choose a reason for hiding this comment

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

Recommended to use sub classes for the events, todo, deadlines

public class Duke {
private static final Scanner SCANNER = new Scanner(System.in);
Copy link

Choose a reason for hiding this comment

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

Why is SCANNER in caps?

Comment on lines 29 to 36
public String getTaskType() {
if (this.taskType.equalsIgnoreCase("todo")) {
return "[T]";
} else if (this.taskType.equalsIgnoreCase("deadline")) {
return "[D]";
} else {
return "[E]";
}
Copy link

Choose a reason for hiding this comment

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

Use inheritance just like we learnt

exitDuke();
break;
}
if (userCommand.equalsIgnoreCase("list")){
Copy link

Choose a reason for hiding this comment

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

Maybe use boolean variables to make this easier to understand

public void markAsDone() {
this.isDone = true;
}

public String getTaskType() {

Choose a reason for hiding this comment

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

looks clean!

}
else if (isValidInput(userCommand)){
processUserRequest(userCommand, inputDetails);
} else {

Choose a reason for hiding this comment

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

new line after bracket


public Task(String description) {
public Task(String description, String taskType) {

Choose a reason for hiding this comment

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

looks good! easy to understand

@@ -80,7 +98,7 @@ private static void listOutTasks() {
while (i < tasksCount) {
Task selectedTask = tasksList[i];
i++;
System.out.println(i + ". " + selectedTask.getStatusIcon() + " " + selectedTask.description);
System.out.println(i + ". " + selectedTask.getTaskType() + selectedTask.getStatusIcon() + " " + selectedTask.description);
}
}

Choose a reason for hiding this comment

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

another blank line here

@@ -1,10 +1,239 @@
import java.io.*;

Choose a reason for hiding this comment

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

You may wish to consider explicitly importing the classes that you wish to use here to increase clarity of what classes are to be maintained here.

Comment on lines 129 to 130
printLine();
printLine();

Choose a reason for hiding this comment

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

It seems you have repeated lines of code here. Consider reviewing what is redundant.

@@ -0,0 +1,13 @@
package Duke;

Choose a reason for hiding this comment

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

Good job in keeping this class simple and easy to read.

protected String taskType;

public Task(String description, String taskType) {
if (taskType.equalsIgnoreCase("event")) {

Choose a reason for hiding this comment

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

This constructor class seems to be lengthy for the start. You could abstract away some details, while ensuring SLAP.

this.isDone = false;
this.taskType = taskType;
}
else if (taskType.equalsIgnoreCase("deadline")) {

Choose a reason for hiding this comment

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

You have several cases here. Instead of multiple else if, perhaps explicit switch case statements could be, See switch layouts: https://se-education.org/guides/conventions/java/basic.html#layout.

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