-
Notifications
You must be signed in to change notification settings - Fork 362
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
[clydelhui] iP #373
base: master
Are you sure you want to change the base?
[clydelhui] iP #373
Conversation
# Conflicts: # src/main/java/Duke.java
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.
Your code is of good quality (though I see some parts that do not conform to the coding standard).
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.
Nice job! Your code is clean and easy to follow overall, and your variables are named meaningfully. You may want to consider adding some Javadocs in the future :) 👍
src/main/java/CommandHandler.java
Outdated
this.taskList = taskList; | ||
} | ||
|
||
public void execute(String command) throws IllegalCommandException, |
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 consider having Javadocs for this method
src/main/java/CommandHandler.java
Outdated
public void execute(String command) throws IllegalCommandException, | ||
IllegalInputException, NumberFormatException { | ||
String[] parsedCommand = command.split(" ", 2); | ||
switch (parsedCommand[0]) { |
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 like your implementation! Perhaps you can consider further abstraction, e.g having an abstract Command class that different types of commands can extend from :)
src/main/java/items/Deadline.java
Outdated
super(description, "D"); | ||
this.endDate = endDate; | ||
} | ||
|
||
public Deadline(String description, boolean done, String endDate) { | ||
public Deadline(String description, boolean done, LocalDate endDate) { |
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 the parameter "done" could be changed to "isDone" 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.
The codes are easy to read and well-functional.
src/main/java/TaskList.java
Outdated
} | ||
|
||
public void delete(int index){ | ||
Task deleted = this.taskList.remove(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.
Can here be some input validity check to ensure index is never less than 1?
@@ -0,0 +1,12 @@ | |||
package dukeexceptions; |
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.
Please conform to the camelNaming rule here to name the package.
Add additional exceptions with better handling.
…mmand and Parser classes.
# Conflicts: # src/main/java/commands/VoidCommand.java
# Conflicts: # src/main/java/elems/TaskList.java
When TaskList methods are called, the ArrayList of Tasks should never be null. Adding assertions allows us to ensure that this invariant is always true. Let's add assertions to the TaskList methods to ensure that all TaskList Objects which are targets for method invocation have non-null ArrayLists at the time of method calling.
execute method in the AddCommmand class is long and can be hard to debug in the future. If more commands are added, the code will get even longer. Let's abstract out the steps taken in each case into a separate method. Future additions of AddCommands can adopt the same style of execution.
add assertions
* 'master' of https://github.com/clydelhui/ip: add assertions
Improve code quality
The user may wish to sort the task list by description to make it easy to read. Let's add a sorting feature to enable the user to do this. Some commands such as list and sort will access the list elements. If the list is empty, they are unable to work as intended. Let's add an exception to show the case when an empty list is being accessed by commands that work with the whole list.
Duke
Duke stores everything for you! It's
GreatTHE ABSOLUTE GREATESTTo experience the greatest of Duke,
Features:
[x] Task management
[ ] Search (coming soon)
If you are a Java programmer, you can take a look at the source code too!. Here is the
main
method in Duke!