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

[Wang Yihe] iP #243

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

Conversation

Yihe-Harry
Copy link

No description provided.

@Yihe-Harry
Copy link
Author

[Wang Yihe] iP

throw new DukeException("Missing component: due date");
}
String time = deadlineArr[1];
if (isDateFormat(time, "yyyy-mm-dd") || isDateFormat(time, "yyyy-m-dd") || isDateFormat(time, "yyyy-mm-d") || isDateFormat(time, "yyyy-m-d")) {
Copy link

Choose a reason for hiding this comment

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

Perhaps might want to add more spaces in front to indent this method to fit with the Java coding convention.


package duke.ui;

import duke.task.*;
Copy link

Choose a reason for hiding this comment

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

It might be a better idea to import relevant files from the task folder instead of importing all?

printLine();
printMessage("Here are the matching tasks in your list");
for (int i = 0; i < task.size(); i++) {
int index = i + 1;
Copy link

Choose a reason for hiding this comment

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

I like how you remember to add spaces around i + 1;

String[] input = temp.split(" ", 2);
Command command = getUserInputType(input[0]);
switch (command) {
case DEADLINE:
Copy link

Choose a reason for hiding this comment

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

"The explicit //Fallthrough comment should be included whenever there is a case statement without a break statement.", quoted from Java coding convection.


import duke.ui.Ui;
import duke.exception.DukeException;
import duke.task.*;
Copy link

Choose a reason for hiding this comment

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

Although all the three classes in task package are used, perhaps listing them out explicitly will comply with the coding standard better? 🤔


public void add(String[] userInput, Ui ui) throws DukeException {
switch (userInput[0]) {
case "todo":
Copy link

Choose a reason for hiding this comment

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

According to our coding standard, perhaps there should not be spaces before each "case"? 🤔

Copy link

@ljhgabe ljhgabe left a comment

Choose a reason for hiding this comment

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

Great work! LGTM except for some naming issues.

}

@Override
public String savedFormat() {
Copy link

Choose a reason for hiding this comment

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

Perhaps the naming here could be even more straightforward? e.g. formatInFile.

task.add(t);
}

public static boolean isDateFormat(String date) {
Copy link

Choose a reason for hiding this comment

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

Good naming for method!

return true;
}

public void parseTask(TaskList task, String info) throws IndexOutOfBoundsException{
Copy link

Choose a reason for hiding this comment

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

If it is a "TaskList", perhaps can rename the variable name to "tasks"? 🤔 (same applies to the rest methods)

return "[ ] " + this.description;
}
}

Copy link

Choose a reason for hiding this comment

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

Perhaps more Javadoc comments could be included? 🤔

User Guide 📣
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