-
Notifications
You must be signed in to change notification settings - Fork 361
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
[Truong Minh Duong] iP #366
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.
Generally, not enough efforts put into CS2103T and not enough codes for review.
I will do the back up one instead.
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.
Other than some minor coding standard issues, the code generally looks neat with good logic 👍🏼
src/main/java/duke/TodoList.java
Outdated
System.out.println("Nice! I've marked this task as done:"); | ||
System.out.println(todo_list.get(index-1)); |
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.
Perhaps you can move System.out.println(..)
into a function in your Ui class to utilise it more, since the purpose of the Ui class is to handle all interactions with the user
@@ -0,0 +1,12 @@ | |||
package duke; | |||
|
|||
public class DukeExceptions extends Exception{ |
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.
While the class name is valid according to coding standards, perhaps it would be more intuitive if it was named DukeException
instead since it only represents an instance of 1 Exception
} | ||
|
||
@Override | ||
public void execute() throws DukeExceptions { todoList.mark(index); } |
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.
Just a small suggestion✌🏼 Since the MarkCommand and UnmarkCommand classes do the same thing except the execute functions maybe you want to consider just using 1 class that handles all marking related commands with a check to determine whether to mark or unmark the task
src/main/java/duke/TodoList.java
Outdated
System.out.println(task); | ||
} | ||
|
||
public int number_of_tasks() { |
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.
Coding standards state the names representing methods must be written in camelCase, so i think getNumberOfTasks
or numberOfTasks
would be better
String[] from_to_timeline = commands[1].split(" /to "); | ||
if (from_to_timeline.length == 1) { |
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.
I've noticed some of your variable names use snake case, however the coding standards states that variable names must be in camelCase, so do remember to check and update your variable names
Delete dead code, change some code to follow coding standard
Complete A-Assertions
Branch a code quality
Add task option duration Add bye command
Complete BCD-Exetension
Duke
Allows you to
Type of tasks
Special features 🤠 :
For developer, can run the
main
method in Duke class: