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

[Yong Kang] iP #283

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

[Yong Kang] iP #283

wants to merge 56 commits into from

Conversation

hyngkng
Copy link

@hyngkng hyngkng commented Aug 24, 2020

No description provided.

Copy link

@bchenghi bchenghi left a comment

Choose a reason for hiding this comment

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

LGTM! The code was really good and followed the coding standard like having private variables and line breaks at operators. Perhaps comments could be added to public methods? Overall, it was really nice.

import java.time.format.DateTimeFormatter;

public class Parser {

Choose a reason for hiding this comment

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

I think public methods should have comments?

String[] parsed = input.split(" ", 2);
String keyword = parsed[0];
String body = "";
if (parsed.length > 1) body = parsed[1];

Choose a reason for hiding this comment

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

I think if else statements should have curly brace?

Copy link

@jeminsieow jeminsieow left a comment

Choose a reason for hiding this comment

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

Overall, the naming of variables and classes are appropriate 👍
Maybe the method names could be more consistent with the coding standard. (Beginning with a verb)

file = new File(filepath);
}

public ArrayList<Task> load() throws FileNotFoundException, DukeException {

Choose a reason for hiding this comment

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

Would a more descriptive method name be more suitable here?

}
}

public void write(TaskList tasks) throws IOException {

Choose a reason for hiding this comment

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

Would a more descriptive method name be more suitable here? Perhaps writeFile?

Comment on lines 82 to 84
private static String desc(String body) {
return body.split(" /", 2)[0];
}

Choose a reason for hiding this comment

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

Perhaps starting the method with a verb? An example can be getDesc.

Comment on lines 94 to 101
private static String deadline(String body) {
String time = body.split(" /by ", 2)[1];
if (time.equals("now")) {
return LocalDate.now().format(DateTimeFormatter.ofPattern("d MMM yyyy"));
} else {
return LocalDate.parse(time).format(DateTimeFormatter.ofPattern("d MMM yyyy"));
}
}

Choose a reason for hiding this comment

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

Perhaps starting the method with a verb? An example can be getDeadline.

Copy link

@itsjerryho itsjerryho left a comment

Choose a reason for hiding this comment

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

I think your code is a very good example of OOP principles and everything is nicely abstracted! Just have to take note of the brackets, other than that I have a lot to learn from your code design!

* @throws DukeException When user input cannot be understood.
*/
public static void parseInput(String input, Ui ui, TaskList tasks, Storage storage) throws DukeException, IOException {
String[] parsed = input.split(" ", 2);

Choose a reason for hiding this comment

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

I didn't know you can do this! Nice 👍

* @return A boolean whether input is bye.
*/
public static boolean isBye(String input){
return input.split(" ", 2)[0].equals("bye");

Choose a reason for hiding this comment

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

just a small suggestion, maybe you can mention that input.split(...)[0] is the firstWord of the input by creating a variable so it's slightly clearer!

noneFound = false;
}
}
if (noneFound) ui.say("No matching tasks.");

Choose a reason for hiding this comment

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

I think you can add in brackets based on the coding standards, but your code is really neat!

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.

None yet

4 participants