-
Notifications
You must be signed in to change notification settings - Fork 364
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
[Eric Leow Yu Quan] iP #98
base: master
Are you sure you want to change the base?
Conversation
This reverts commit 2b287c3.
…wishes to see the current list
…ut in order when requested by the user
… added by the user
…been marked as completed
…er adding a valid task
…their separate descriptions and timings
…about previously added information
…t and printing out in MMM dd yyyy format using the LocalDate class
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.
LGTM! Except the formatting of your javadocs might need to follow the coding standard
|
||
/** | ||
* Returns the string representation of a Deadline instance in MMM d yyyy format. | ||
* @return the desired string representation of a Deadline instance. |
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 should have an empty line between description and parameter section?
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.
Thanks for pointing that out! I wasn't aware of this coding standard
} | ||
|
||
/** | ||
* Returns the string representation of a Deadline instance in YYYY-MM-DD format. |
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 might have missed a spacing between the descriptions and parameter section here as well.
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 is pretty clean aside from those few nits. Nice job! 👍
src/main/java/duke/TaskList.java
Outdated
*/ | ||
public String printContents() { | ||
String response = "Attempting to print out your tasks...\n"; | ||
if (lstOfItems.size() == 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.
You might have added an extra space after "0".
src/main/java/duke/TaskList.java
Outdated
} | ||
|
||
/** | ||
* A method stub to test the TaskList class. |
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 may consider starting the method header with an active verb.
Eg. "Tests the TaskList class."
import javafx.stage.Stage; | ||
|
||
|
||
public class Duke extends Application { |
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 may consider adding JavaDoc comments for your classes too!
if (currentTask.charAt(1) == 'T') { | ||
task = new Todo(currentTask.substring(7)); | ||
} else if (currentTask.charAt(1) == 'D') { | ||
String[] split = currentTask.split("by: "); |
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 consider renaming split
into a plural form since it is a String array.
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 LGTM, I really like the way you commented your code and gave credit to your sources!
src/main/java/duke/Ui.java
Outdated
public String instruct() { | ||
String instructString = "There are various commands you can instruct me to do.\n"; | ||
instructString += ("A. If you want to see your list of tasks, type 'list'\n"); | ||
instructString += ("B. If you want to add a task, there are three possible commands:\n"); | ||
instructString += ("\t1. Type 'todo (name of task)' to add a todo task.\n"); | ||
instructString += ("\t2. Type 'deadline (name of task) /by (date in YYYY-MM-DD)\n"); | ||
instructString += (" to add a task that is to be done by a certain date.\n"); | ||
instructString += ("\t3. Type 'event (name of task) /from (date and time) /to "); | ||
instructString += ("(date and time) to add in a time-limited event.\n"); | ||
instructString += ("C. To delete a task, type 'delete X' where X is the task number\n"); | ||
instructString += ("D. To mark a task as completed, type 'mark X'\n"); | ||
instructString += ("E. To save and exit, type 'bye'\n"); | ||
return instructString; | ||
} |
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 way you formatted your paragraph line by line. Perhaps you could look into using StringBuilder here.
src/main/java/duke/TaskList.java
Outdated
|
||
public class TaskList { | ||
|
||
private ArrayList<Task> lstOfItems; |
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.
private ArrayList<Task> lstOfItems; | |
private ArrayList<Task> lstOfTasks; |
* Marks the task as completed. | ||
* It does not matter whether the task has previously been marked as completed. | ||
*/ | ||
public void makeCompleted() { |
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 like markAsCompleted
can be used here?
* Adds and saves the user's tasks into the File at the filePath. | ||
* @param tasks the user's list of tasks before termination of the chatbot. | ||
*/ | ||
public void addToFile(TaskList 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.
Any reason why you named it as such instead of save
(like load and save)?
Lack of documentation for constructors that may make it difficult to understand certain parameters if not explained Explaining the parameters used in all constructors allows for easier appreciation of the purpose of each class, and increases ease in bug- fixing in the future as it is easier to pinpoint which class or attribute is responsible for the bug
Some coding standard violations such as not leaving a line in JavaDocs between the description and the @params and @return statements Made changes accordingly to fit the guidelines for standardization, to ensure that JavaDocs written adhere to these standards and are of high quality, and to maximize learning. Names of some variables were also changed to better represent what items they were actually storing to avoid any misconceptions.
Clean up code with the aid of comments given by peers and improve code to adhere to code quality guidelines
No assertions present that could lead to instances where a code does not work properly as intended, such as attempting to access a list that was not even created Add assertions that kept track of assumptions made in code, to check if these assumptions are held when certain parts of the code is run, to avoid undefined behaviour or errors
Add assertions to key parts of code
to allow saving and loading of previous tasks involving their respective priority levels. There was no ability to save previous priority levels, causing all tasks to have low priority level every time the application is launched Add ability to toggle priority level incrementally (among the 3 possible priority levels). Allow saving of data to save user time and to ensure that users do not need to set priority level of tasks manually each time.
Long methods such as run() in Duke class and manual checking of user's input for certain words resulted in arrow-head code and overly long methods. Manual checking of input was abstracted into more methods in the Parser class, with a checking of valid integers. Code is cleaner and easier to follow.
methods implemented, such as unmark task and find keyword
Duke is a bot that can help you manage your to-do list and keep them in one place!
trivialimportant like your other half's birthday? 😍You're not alone! Scientists show that memory is much more transient than you may think
Features:
Strengths
If you are a Java programmer, you can use it to practise Java too. Here's the
main
method: