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 Ri Zhao] iP #116

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

Conversation

therizhao
Copy link

No description provided.

damithc and others added 30 commits July 23, 2020 23:27
Type of tasks include
- event
- deadline
- todo
Type of tasks include
- event
- deadline
- todo
Copy link

@li-s li-s left a comment

Choose a reason for hiding this comment

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

LGTM!

src/main/java/Launcher.java Outdated Show resolved Hide resolved
if (cmd.isExit()) {
break;
}
} catch (DukeException e) {
Copy link

Choose a reason for hiding this comment

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

Could the exception e be mroe descriptive?

Copy link
Author

Choose a reason for hiding this comment

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

I'm not too sure what do you mean here 😅

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, I don't get what you mean here. 😅

throw Ui.taskDateEmptyException(super.getCommandType());
}

String name = taskParts[0];
Copy link

Choose a reason for hiding this comment

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

What does name refer to here? the command (todo, deadline), or the description of the task?

Copy link
Author

@therizhao therizhao Sep 7, 2020

Choose a reason for hiding this comment

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

It refers to the name (aka description) of the deadline task. I think the naming could be more descriptive, e.g. deadlineName. But I am using just name for the name, so that it is consistent with the rest of the commands.

Copy link

@TheSpaceCuber TheSpaceCuber left a comment

Choose a reason for hiding this comment

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

LGTM! Great use of OOP.

README.md Show resolved Hide resolved
src/main/java/duke/command/Command.java Show resolved Hide resolved
src/test/java/duke/command/CommandTest.java Show resolved Hide resolved
src/main/java/duke/parser/Parser.java Show resolved Hide resolved
…-Quality

refactor(A-CodeQuality): Extract cli logic from duke class to CliLauncher class
feat(C-Tagging): Add task tagging feature
fix: Fix Gui to be able to write tasklist into file
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

3 participants