Skip to content

[Ma Jiajun] iP#197

Open
Jamarcus111 wants to merge 24 commits into
nus-cs2113-AY2324S2:masterfrom
Jamarcus111:master
Open

[Ma Jiajun] iP#197
Jamarcus111 wants to merge 24 commits into
nus-cs2113-AY2324S2:masterfrom
Jamarcus111:master

Conversation

@Jamarcus111

Copy link
Copy Markdown

No description provided.

@aditichadha1310 aditichadha1310 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, good job!

@@ -0,0 +1,40 @@
public class TaskFactory {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Avoid long methods. Take corrective action when the method exceeds 30 lines of code.

Comment thread src/main/java/CommandParser.java Outdated
throw new HandleException("The input cannot be empty!");
} else if (userInput.equalsIgnoreCase("list")) {
taskList.listTasks();
} else if (!userInput.startsWith("todo") && !userInput.startsWith("deadline") && !userInput.startsWith("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.

This is a fairly complex statement. You might want to break this into sub-expressions and store values into some boolean variables to check for the "if" condition.

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

public void addTask(String userInput) throws HandleException {
// Similar to addTask method in TaskManager, but refactored for this class

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Make sure the comments are for the reader and not private notes for yourself. It should help in increasing code readability.

String type = parts[0];
Task task = null;

if (parts.length < 2 || parts[1].isEmpty()) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Avoid the use of Magic numbers.

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.

2 participants