Skip to content
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

[Yap Jia Aun] Duke Increments #351

Open
wants to merge 68 commits into
base: master
Choose a base branch
from

Conversation

justarock111
Copy link

Duke Increments

Copy link

@Q-gabe Q-gabe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, great job! I think OOP design is pretty solid so far and things are pretty clean, besides minor issues with style, you did a great job imo. LGTM! 👍

* Represents user's 'deadline' commmand to chatbot.
* The 'DeadlineCommand' class supports operators (i) executing the command
* and (ii) checking if the bot has exited its conversation with the user(in superclass).
*/
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very thorough javadoc descriptions!

try {
Date twentyFourHourFormat = new SimpleDateFormat("dd/MM/yyyy HHmm").parse(deadline);
String twelveHourFormat = new SimpleDateFormat("dd/MM/yyy hh:mm").format(twentyFourHourFormat);
this.deadline = new SimpleDateFormat("dd/MM/yyy hh:mm").parse(twelveHourFormat);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps another way of doing it would be to just use:
this.deadline = twentyFourHourFormat;
This prevents the need for that additional parse back from twelveHourFormat back to the Date object again.


/**
* Executes the command and print out respecive reponse.
*
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small typos here haha.
* Excutes the command and print out respective response.

* @return boolean of whether program has exited
*/
public abstract boolean isExit();
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding newlines at the end of your files, as it is a general convention. It doesn't really cause any issues beside making GitHub and checkstyle whine, but there is a reason behind doing so, https://stackoverflow.com/questions/729692/why-should-text-files-end-with-a-newline. Hope this gives some insight!

taskList.markTaskDone(taskNum);
Task updatedTask = taskList.getTask(taskNum);
storage.updateText(taskNum);
storage.updateText(taskNum);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason why updateText here is called twice?


default:
return determineInputType(input);

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is neat! I didn't think of separating input commands vs. bye and list commands. 👍

*/
public static Command determineInputType(String input) throws DukeException {
if (input.contains("done")) {

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would be careful with using input.contains() because for cases where the input is, for example:
todo select deadline for goals
Or conversely:
deadline do up a todo list /by 12/12/19 12:30
Your code might then not work as intended. You can consider instead using the startsWith() function (https://www.tutorialspoint.com/java/java_string_startswith.htm). 😃

/**
* List of tasks.
*/
private ArrayList<Task> todoList;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a small semantic nitpick but a todoList might imply a list of todo tasks instead a list of tasks. I totally understand this however. 😅

*/
public String getStatusIcon() {
return (this.isDone ? "v" : "x"); // "\u2713" : "\u2718"); //return tick or X symbols
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned previously in my reply to your comments, if the alt codes there don't work do let me know! I'm curious as to why it doesn't work as well. 😃

Copy link

@fadisng fadisng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general very clean code and easy to read, good job!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants