Skip to content

[Huang Zhenxin] ip#249

Open
Hzxin wants to merge 69 commits into
nus-cs2103-AY2021S2:masterfrom
Hzxin:master
Open

[Huang Zhenxin] ip#249
Hzxin wants to merge 69 commits into
nus-cs2103-AY2021S2:masterfrom
Hzxin:master

Conversation

@Hzxin
Copy link
Copy Markdown

@Hzxin Hzxin commented Feb 1, 2021

No description provided.

public class AddCommand extends Command {

/**
* Create a AddCommand object for execution.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Perhaps it is 'Creates' instead of 'Create'?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

... I noticed the same minor grammatical issue in other places too



private static Boolean handleDeadline(String task, String date) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Should this new line be removed?

Comment on lines +13 to +14


Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Should these lines between class declaration and constructor be removed?

public class DeleteCommand extends Command {


public DeleteCommand(String task, String date) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Perhaps Javadoc comments could be added for the constructor?

/**
* Sub-class of command that represents and execute the "bye" instruction of user.
*/
public class ExitCommand extends Command {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I like how you further extract Command into child classes :)


import duke.driver.DukeDriver;


Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Should there be a new line here?

TaskList.addTask(deadlines);
System.out.println(Ui.biggerBox(deadlines));
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Some of the new line spacing is a bit inconsistent. Perhaps you can standardize whether you're putting a new line spacing after every curly brace

* @param date date of the user task to be done.
*/
public FindCommand(String task, String date) {
super("find", task, date, command -> {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I like how you split up the code into separate lines! Much more readable

Comment thread src/main/java/duke/task/Deadlines.java Outdated
/**
* sub-class of Task to represents a task with deadline.
*/
public class Deadlines extends Task {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Perhaps this can be named Deadline instead of Deadlines, since it is a single Task

Comment thread src/main/java/duke/task/TaskList.java Outdated
}
}

public static void clearAllTask() {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Shouldn't this be clearAllTasks instead?

Comment thread src/main/java/duke/ui/Ui.java Outdated
* @param t task to be wrapped.
* @return the String representation of the task name wrapped in a chatBox.
*/
public static String biggerBox(Task t) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Perhaps can rename this to makeBiggerBox

* @param date the String representation of date.
* @return MMM d yyyy format of date and return error message if the given date is in wrong format.
*/
public static String converter(String date) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Should name this as an action, like convert

@@ -0,0 +1,103 @@
package duke.ui;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I like how everything is packaged! Very neat and clear :)

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants