Skip to content

[Yan Zaiya] iP#211

Open
skyanzy wants to merge 25 commits into
nus-cs2113-AY2223S2:masterfrom
skyanzy:master
Open

[Yan Zaiya] iP#211
skyanzy wants to merge 25 commits into
nus-cs2113-AY2223S2:masterfrom
skyanzy:master

Conversation

@skyanzy

@skyanzy skyanzy commented Feb 1, 2023

Copy link
Copy Markdown

No description provided.

Comment thread src/main/java/Task.java Outdated
this.isDone = false;
}

//...

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 could remove this comment

Comment thread src/main/java/Duke.java Outdated
System.out.println(greet);
boolean shouldContinue = true;
Scanner in = new Scanner(System.in);
//String[] tasks = new String[100];

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 remove this comment if you are no longer using it

Comment thread src/main/java/Duke.java Outdated
}
System.out.println(DIVIDER_LINE);
}else if (action.startsWith("mark")){
int dividerPos = action.indexOf(" ");

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 could spell out "position" or change it to "Index"

Comment thread src/main/java/Duke.java Outdated
Comment on lines +40 to +42
System.out.println(DIVIDER_LINE + "Nice! I've marked this task as done:\n"
+ tasks[toBeMarked].getStatusIcon() + " " + tasks[toBeMarked].description
+ "\n" + DIVIDER_LINE);

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 print statement is a bit lengthy. Perhaps you could use the ToString override method in your classes to shorten it

Comment thread src/main/java/Duke.java Outdated
Comment on lines +13 to +16
String greet = DIVIDER_LINE
+ "Hello! I'm Duke\n"
+ "What can i do for you\n"
+ DIVIDER_LINE;

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 combining the welcome statement into one string

@Thiolk Thiolk 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 code quality is great although you could consider using switch statements and shortening the print statements to improve the readability of the code.

Comment thread src/main/java/Duke.java Outdated
Comment on lines +36 to +50
}else if (action.startsWith("mark")){
int dividerPos = action.indexOf(" ");
int toBeMarked = Integer.parseInt(action.substring(dividerPos + 1)) - 1;
tasks[toBeMarked].markAsDone();
System.out.println(DIVIDER_LINE + "Nice! I've marked this task as done:\n"
+ tasks[toBeMarked].getStatusIcon() + " " + tasks[toBeMarked].description
+ "\n" + DIVIDER_LINE);
}else if (action.startsWith("unmark")) {
int dividerPos = action.indexOf(" ");
int toBeUnmarked = Integer.parseInt(action.substring(dividerPos + 1)) - 1;
tasks[toBeUnmarked].markAsUndone();
System.out.println(DIVIDER_LINE + "Nice! I've marked this task as undone:\n"
+ tasks[toBeUnmarked].getStatusIcon() + " " + tasks[toBeUnmarked].description
+ "\n" + DIVIDER_LINE);
}else {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Should there be a spacing between the curly brackets and the "else if" and the "else"?

Comment thread src/main/java/Duke.java Outdated
+ tasks[toBeMarked].getStatusIcon() + " " + tasks[toBeMarked].description
+ "\n" + DIVIDER_LINE);
}else if (action.startsWith("unmark")) {
int dividerPos = action.indexOf(" ");

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 could consider writing your variable name in full?

Comment thread src/main/java/Duke.java Outdated
Comment on lines +32 to +42
System.out.println(Integer.toString(i + 1) + "." + "" +tasks[i].getStatusIcon()
+ " " +tasks[i].description);
}
System.out.println(DIVIDER_LINE);
}else if (action.startsWith("mark")){
int dividerPos = action.indexOf(" ");
int toBeMarked = Integer.parseInt(action.substring(dividerPos + 1)) - 1;
tasks[toBeMarked].markAsDone();
System.out.println(DIVIDER_LINE + "Nice! I've marked this task as done:\n"
+ tasks[toBeMarked].getStatusIcon() + " " + tasks[toBeMarked].description
+ "\n" + DIVIDER_LINE);

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 in ensuring that the character count of each line should not exceed 120 characters.

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

@Override
public String toString(){
return getTypeIcon() + super.getStatusIcon() +super.description + " (by: " + this.by + ")";

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Be consistent with the space around +

Comment thread src/main/java/Duke.java Outdated
public class Duke {
public static String DIVIDER_LINE = "______________________________\n";

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.

Your main method feels a bit long, can consider modularizing it. According to the textbook, the recommendation is to have 30 LOC.

Comment thread src/main/java/Duke.java Outdated
taskCount += 1;
printNumTask(taskCount);
}else if (action.startsWith("event")) {
int dividerPosition1 = action.indexOf("/from");

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You repeat this line of code quite often. Consider creating a method and passing the respective variable of /from or /by

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