-
Notifications
You must be signed in to change notification settings - Fork 73
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
[Alexander Lim] iP #74
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, you have abided to the coding standard. There is a slight violation in terms of the if-else statement. You may refer to the individual comment on that. Good implementation of code quality. Maybe, for a slight improvement, you can add SLAP based on the individual comment that I have added!
src/main/java/Duke.java
Outdated
while (true) { | ||
userInput = in.nextLine(); | ||
if (userInput.equals("bye")){ | ||
break; | ||
} | ||
else if (userInput.equals("list")){ | ||
listTasks(tasks, count); | ||
} | ||
else if (userInput.startsWith("done")){ | ||
markTaskAsDone(tasks, userInput); | ||
} | ||
else if (userInput.startsWith("todo")){ | ||
count = addTodoTask(userInput, tasks, count); | ||
} | ||
else if (userInput.startsWith("deadline")){ | ||
count = addDeadlineTask(userInput, tasks, count); | ||
} | ||
else if (userInput.startsWith("event")){ | ||
count = addEventTask(userInput, tasks, count); | ||
} | ||
else { | ||
count = addGeneralTask(userInput, tasks, count); | ||
} | ||
} | ||
System.out.println("Bye. Hope to see you soon!"); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you can summarise this in a method in order to add SLAP to your main function. Also, in terms of coding quality, you may want to look up the coding standard for if-else statement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a glaring coding standard violation in this code :-p
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I've noticed that I have made a mistake in the coding standard. Thanks for your input!
src/main/java/Duke.java
Outdated
@@ -39,21 +32,29 @@ else if (userInput.startsWith("event")){ | |||
count = addEventTask(userInput, tasks, count); | |||
} | |||
else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be clearer if you can consider to wrap these if-else statements into "switch" cases
src/main/java/Duke.java
Outdated
System.out.println("Hey! I am " + logo); | ||
System.out.println("What would you like to do?"); | ||
displayWelcome(logo); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These three codes seem that they can be all wrapped into "displayWelcome(logo)"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thats a good idea, I will implement it into my code :)
src/main/java/Duke.java
Outdated
for (int j = 0; j<count; j++) { | ||
System.out.println(j+1 + ". " + tasks[j]); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In terms of coding standard, it would be better if you can consider to keep space consistency, i.e. for (int j = 0; j < count; j++)...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well noted! 👍
src/main/java/Duke.java
Outdated
System.out.println("Nice! I've marked this task as done:"); | ||
System.out.println(" " + tasks[itemNumber-1]); | ||
markTaskAsDone(tasks, userInput); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be better if you can consider to wrap these "println"s into markTaskAsDone; similar suggestion for other command case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your suggestion 👍
Add Increment: Level-8
No description provided.