-
Notifications
You must be signed in to change notification settings - Fork 461
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
[Isaac] iP #496
base: master
Are you sure you want to change the base?
[Isaac] iP #496
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, your code looks good and I really like how you organized it nicely into different packages, especially commands
! There's a couple of minor code quality / coding standard issues below, but other than that, LGTM!
|
||
import java.time.format.DateTimeFormatter; | ||
|
||
public abstract class Command { |
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 the use of an abstract class here, as well as your great organization in the commands
package!
} | ||
} | ||
|
||
public void store(TaskList toStore) { |
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 could consider writing javadocs for these more complicated public facing methods, to give others a clearer picture of what they do?
|
||
@Override | ||
public void execute(TaskList taskList, UI ui, Storage storage) { | ||
String taskDesc = input.substring(5); |
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 a named constant instead of a magic number would make the code here (and other parts) clearer?
private final int taskID; | ||
|
||
public DeleteTaskCommand(String input) throws DukeException { | ||
if (!checkValid(input)) throw new DukeException(" ☹ OOPS!!! Please enter task to delete."); |
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.
Should the conditional here be wrapped in curly brackets?
return taskList.size() == 0; | ||
} | ||
|
||
public int size() { |
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.
Should this method name be a verb? e.g. getSize()
if (isEmpty()) { | ||
throw new DukeException(" ☹ OOPS!!! Seems like your list is empty."); |
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 that you thought of & handled this edge 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.
Other than some minor code standard issues, the overall code looks good and is ready to merge! I really like the use of OOP to abstract out the common functionalities as well!
@@ -0,0 +1,44 @@ | |||
package Duke.commands; |
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.
You may want to consider changing names representing packages to all lowercase, to conform to the coding standard.
src/main/java/Duke.java
Outdated
} | ||
} | ||
|
||
public void run() { |
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 really like how all the detailed functionalities of Duke.run() are abstracted further into classes.
@@ -0,0 +1,66 @@ | |||
package Duke.storage; | |||
|
|||
import Duke.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.
You may want to consider avoiding wildcard imports
import Duke.commands.*; | ||
import Duke.exceptions.DukeException; | ||
|
||
public class Parser { |
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 how you abstracted the functionalities here into separate Command classes
|
||
public class MarkTaskCommand extends Command { | ||
|
||
private final int taskID; |
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 think taskID can be written as taskId instead to conform to the camelCase standard
Duke uses terminal to receive inputs and display outputs. This interface is ugly and not user friendly. New GUI was implemented to make Duke appear in chat form, much more friendly.
CodeQuality Branch
Undo has some bugs It does not work as intended after some testing The implementation was changed to fix the bugs
GUi is lacktuster Made several improvements (added profile picture, etc...) Fixed several bugs from previous code
This reverts commit 5f7ef7b.
Duke
“Productivity is never an accident. It is always the result of a commitment to excellence, intelligent planning, and focused effort.” – Paul J. Meyer (source)
Duke tracks your tasks for you so your mind is free to focus on the tasks at hand. It's,
quickLIGHTNING FAST in response timeAnd it is FREE!
Features:
If you Java programmer, you can use it to practice Java too. Here's the main method:
public class Main { public static void main(String[] args) { new Duke(data/tasks.txt).run(); } }