-
Notifications
You must be signed in to change notification settings - Fork 228
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
[Jared Lim] iP #276
base: master
Are you sure you want to change the base?
[Jared Lim] iP #276
Conversation
Added ArrayList to store input.
src/main/java/Duke.java
Outdated
|
||
/** | ||
* Main class to take in user input. | ||
* @param args Filler |
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 meaningful name for @param could be possibly replaced here
src/main/java/Event.java
Outdated
} | ||
|
||
/** | ||
* Overriden constructor to account for deadline and events that have timestamps. |
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.
Not too sure if "Overriden" is the appropriate term here. I think "Overloaded constructor" may be more appropriate.
src/main/java/Duke.java
Outdated
} | ||
|
||
/** | ||
* Enumerates all tasks in the lis using 1-based indexing. |
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.
Minor typo for "list"
* @param description Description of method | ||
* @param eventDate Boolean flag indicating whether task is done | ||
*/ | ||
public Task(String description, String eventDate) { |
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.
Might be a good idea to add "@OverRide" tag if this method is truly overriden
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 generally very clean and easy to read! i liked how your method names were clear and how you separated long lines accurately :)
|
||
/** | ||
* Driver class that takes in input and performs certain functions based on input. | ||
*/ | ||
public class Duke { |
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.
this class is rather long, perhaps you could consider abstracting the methods out?
src/main/java/Duke.java
Outdated
} else if (command.equals("list")) { | ||
enumerateTasks(); | ||
} else if (command.startsWith("done")) { | ||
String[] delString = command.split("\\s+"); |
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.
delString is not a very intuitive variable name, maybe you could make it more specific?
src/main/java/Event.java
Outdated
} | ||
|
||
/** | ||
* Overriden constructor to account for deadline and events that have timestamps. |
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 the term you meant to use might be "overloaded" instead
src/main/java/Task.java
Outdated
low, | ||
average, | ||
high |
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 that constants in enum should be in all uppercase, eg "LOW", "AVERAGE" and "HIGH"
src/main/java/Task.java
Outdated
} | ||
|
||
/** | ||
* Overriden method to mark a task as 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.
Did you mean "Overloaded" here as well? I've noticed it in a few other places. Otherwise, as the other reviewer mentioned, overridden methods should tagged with "@OverRide"
Included the gradle wrapper and the associated dependencies for the Junit test. Added two JUnit Test: One to test the loading of the stored text file, the other to test marking as done utility. Both under src/test/java.
Applied changes to all other dependent classes as well.
…checks for non empty objects, and list to avoid unchecked null pointers which can crash the program.
…s as much as possible by abstracting it away into the Task classes.
…ption to specify type of search: partial case sensitive, partial non- case sensitive, full exact match.
GUI echoes user commands, returns output by bot.
…methods which are quite clear.
…method body. Prevented as much as possible the use of magic strings when it comes to important kinds of data. Avoided direct usage of list indexing in Duke method by abstracting to the TaskList method.
…nsistencies for specific inputs like missing spaces.
This reverts commit 518584e.
No description provided.