Skip to content

[Chan Yong Soon, Kendrew] iP#458

Open
KendrewChan wants to merge 53 commits into
nus-cs2103-AY2021S1:masterfrom
KendrewChan:master
Open

[Chan Yong Soon, Kendrew] iP#458
KendrewChan wants to merge 53 commits into
nus-cs2103-AY2021S1:masterfrom
KendrewChan:master

Conversation

@KendrewChan
Copy link
Copy Markdown

No description provided.

Copy link
Copy Markdown

@BobbyZhouZijian BobbyZhouZijian left a comment

Choose a reason for hiding this comment

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

LGTM. Just some nits to fix.

Comment thread src/main/java/Duke/Launcher.java
Comment thread src/main/java/Duke/Main/Duke.java
Comment thread src/main/java/Duke/Main/Duke.java
Comment thread src/main/java/Duke/Main/Duke.java
Comment thread src/main/java/Duke/Main/Parser.java
Comment thread src/main/java/Duke/Main/Ui.java Outdated
Comment thread src/main/java/Duke/Main/Ui.java Outdated
Comment thread src/main/java/Duke/Tasks/Deadline.java
Comment thread src/main/java/Duke/Tasks/Task.java
Comment thread src/main/java/Duke/fxml/DialogBox.java
Copy link
Copy Markdown

@zyifanz zyifanz left a comment

Choose a reason for hiding this comment

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

Excellent work! Overall LGTM 👍

Comment thread src/main/java/Duke/Main/Ui.java Outdated
Comment thread src/main/java/Duke/Main/Parser.java
Comment thread src/main/java/Duke/Main/TaskList.java
Comment thread src/main/java/Duke/Main/TaskList.java
@KendrewChan KendrewChan closed this Sep 5, 2020
@damithc
Copy link
Copy Markdown
Contributor

damithc commented Sep 7, 2020

This PR should be kept open

@damithc damithc reopened this Sep 7, 2020
@KendrewChan
Copy link
Copy Markdown
Author

KendrewChan commented Sep 7, 2020 via email

Copy link
Copy Markdown

@therizhao therizhao left a comment

Choose a reason for hiding this comment

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

Hello Kendrew, I am writing this review for my tutorial class 😄

/**
* Sets Status of Task to be done or not.
*
* @param checked String "0" to represent false, and "1" for true
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hello 😄 . Could you use a boolean for the property done instead? If there are more than 2 states, you could consider using a enum 🔢 . Otherwise all good 👍

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.

5 participants