Skip to content

CerIsaiah iP#81

Open
CerIsaiah wants to merge 13 commits into
nus-cs2113-AY2324S1:masterfrom
CerIsaiah:master
Open

CerIsaiah iP#81
CerIsaiah wants to merge 13 commits into
nus-cs2113-AY2324S1:masterfrom
CerIsaiah:master

Conversation

@CerIsaiah

Copy link
Copy Markdown

No description provided.

Comment thread src/main/java/Duke.java Outdated
System.out.println("Congrats I marked this class as done : " + markedTask.getDescription());
markedTask.isDone = true;
System.out.println("Congrats I marked this class as done : " + markedTask.getDescription());
} catch (Exception ArrayIndexOutOfBoundsException){

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 you catch the exception during the input phase? Check if 0 < user input index < num of items + 1

Comment thread src/main/java/Duke.java Outdated
System.out.println(todo.getDescription());
tasks.add(todo);

} catch (Exception e) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

what types of errors are expected? can the exception catch be more specific?

Comment thread src/main/java/Todo.java
@@ -0,0 +1,12 @@
public class Todo extends Task{
protected String 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.

should this variable be marked as private instead of protected? its the final class in the inheritance chain

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

Nice work, I have only suggested a small change to improve coding style

Comment thread src/main/java/Duke.java Outdated
Comment on lines +24 to +76
if (input.equals("list")){
int numOfTasks = 0;

for (Task task: tasks){
numOfTasks++;
System.out.println(numOfTasks + ": " + task.getDescription());
}

} else if (input.toLowerCase().startsWith("mark ")){
String[] parts = input.split(" ");

int taskNum = Integer.parseInt(parts[1]);
try {
Task markedTask = tasks.get(taskNum - 1);

markedTask.isDone = true;
System.out.println("Congrats I marked this class as done : " + markedTask.getDescription());
} catch (Exception ArrayIndexOutOfBoundsException){
System.out.println("That task doesnt exist");
}

} else if (input.toLowerCase().startsWith("unmark ")) {
String[] parts = input.split(" ");

int taskNum = Integer.parseInt(parts[1]);
tasks.get(taskNum - 1).isDone = false;

try {
Task unmarkedTask = tasks.get(taskNum - 1);

unmarkedTask.isDone = false;
System.out.println("I unmarked this class as done: " + unmarkedTask.getDescription());
} catch (Exception ArrayIndexOutOfBoundsException){
System.out.println("That task doesnt exist");
}
} else if (input.toLowerCase().startsWith("deadline")){
try {
String[] toDoSplit = input.split("/");
//First part is task, and last is when by
String desc = toDoSplit[0].substring(9).trim(); // removes "deadline
Deadline deadline = new Deadline(desc, toDoSplit[1].trim());
System.out.println(deadline.getDescription());

tasks.add(deadline);

} catch (Exception ArrayIndexOutOfBoundsException){
System.out.println("Put a / after your task if you want to add a todo");
}


} else if (input.toLowerCase().startsWith("event")){
try {
String[] toDoSplit = input.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.

Should you use a switch() statement instead of multiple if else?

Comment thread src/main/java/Duke.java Outdated
break;
}

if (input.equals("list")){

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 there a reason why you don't use toLowerCase() for this specific command?

@YongbinWang YongbinWang 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 work. Just some minor nitpicks from my part. Keep up the good work!

Comment thread src/main/java/Event.java
Comment on lines +4 to +5
String start;
String end;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Consider adding in access modifiers and not leave it to default.

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

} else if (input.toLowerCase().startsWith("deadline")){
try {
String[] toDoSplit = input.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.

Consider changing the variable name to a more suitable plural name.

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