-
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
[Fong Yih Jie] iP #487
base: master
Are you sure you want to change the base?
[Fong Yih Jie] iP #487
Conversation
Add Gradle support
# Conflicts: # src/main/java/duke/Storage.java # src/main/java/duke/TaskList.java # src/main/java/duke/Ui.java
# Conflicts: # src/main/java/duke/Parser.java # src/main/java/duke/TaskList.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.
Overall, the PR was pretty good, and adhered to the style guide! There were only some minor issues that I would like you to go over. Good job!
src/main/java/duke/Duke.java
Outdated
protected TaskList tasks; | ||
protected Parser parser; | ||
|
||
private boolean runningDuke; |
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.
Would a proposition be a clearer name for this boolean? (e.g. dukeIsRunning
?)
src/main/java/duke/Parser.java
Outdated
* @param verbose Boolean to indicate verbosity of Ui. | ||
* @throws DukeException If command cannot be parsed or is invalid. | ||
*/ | ||
public void parse(String command, boolean verbose) throws DukeException { |
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.
Would a proposition be a clearer name for the boolean parameter? (e.g. isVerbose
)?
src/main/java/duke/Parser.java
Outdated
* @throws DukeException If command cannot be parsed or is invalid. | ||
*/ | ||
public void parseTwo(String command) throws DukeException { | ||
String[] split = command.split(" ", 2); |
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.
Would a plural name be a better name for an array?
src/main/java/duke/Parser.java
Outdated
String[] split = command.split(" ", 2); | ||
String commandName = split[0]; | ||
int id = tasks.getSize(); | ||
if (split.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.
Nothing major, but whitespace between ){
would be nice! May I suggest running checkStyle
before submitting, it has made my life a lot easier!
src/main/java/duke/Parser.java
Outdated
* @throws DukeException If command cannot be parsed or is invalid. | ||
*/ | ||
public void parseFind(String command) throws DukeException { | ||
String[] split = command.split(" ", 2); |
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.
Same as above
src/main/java/duke/TaskList.java
Outdated
* Class that stores and manipulates tasks for Duke Bot. | ||
*/ | ||
public class TaskList { | ||
protected final ArrayList<Task> taskList; |
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.
Would a name like tasks
be more concise?
src/main/java/duke/task/Task.java
Outdated
/** | ||
* Marks current Task as done. | ||
*/ | ||
public void done() { |
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.
Will a verb be more apt for this method name?
src/main/java/duke/task/Task.java
Outdated
/** | ||
* Marks current Task as not done. | ||
*/ | ||
public void undone() { |
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.
Same as above
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.
A few nits when naming variables, nothing major, should be good to merge when changed. 👍
src/main/java/duke/Ui.java
Outdated
+ "| |_| | |_| | < __/\n" | ||
+ "|____/ \\__,_|_|\\_\\___|\n"; | ||
private Scanner sc; | ||
private boolean verbose; |
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 more intuitive variable name here?
*/ | ||
public EventTask(String description) throws DateTimeParseException, DukeException { | ||
super(); | ||
this.commandString = description; |
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 an overloaded constructor for superclass Task can be looked into
src/main/java/duke/Storage.java
Outdated
* Class that handles loading and saving of tasks for Duke Bot. | ||
*/ | ||
public class Storage { | ||
private FileWriter output; |
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 more intuitive name could be used, that suggests the attribute is a filewriter
Included some assertions in Parser, Task and Storage class. These assertions should help to flag out bugs in these classes during runtime.
Improved on quality of code according to code quality guide. Removed instances of magic number and revised bad variable naming. This makes the code easier to read.
Add assertions
# Conflicts: # src/main/java/duke/Storage.java
Branch a code quality
Used a HashSet to store task strings for duplicate checking. Checking for duplicate via a HashSet is faster than checking for duplicates by iterating through an ArrayList.
Find command used to search using toString method of the tasks. This will include (by:...) and (at:...) in the search. This will mean that "find y" will return all deadlines and "find (" will return all deadlines and events.
Storage used to save only after bye command. Now save after every modification to task list. Also updated storage file chick.txt to not store uncheck command as it is redundant. Duplicate check was missing in the implementation of removing the task from duplicate check after deleting the task from task list.
Storage used to save only after bye command. Now save after every modification to task list. Also updated storage file chick.txt to not store uncheck command as it is redundant. Duplicate check was missing in the implementation of removing the task from duplicate check after deleting the task from task list.
Changed backslash in userguide to forward slash.
DukePro
DukePro frees your mind of having to remember things you need to do. It's,
FASTSUPER FAST to useAll you need to do is,
And it is FREE!
Features:
If you Java programmer, you can use it to practice Java too. Here's the
main
method: