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

[Wen Junhua] iP #20

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

[Wen Junhua] iP #20

wants to merge 189 commits into from

Conversation

Jh123x
Copy link

@Jh123x Jh123x commented Aug 20, 2020

No description provided.

damithc and others added 30 commits July 23, 2020 23:27
Changelog:
1. Changed flavour of duke
2. Added correct Expected output
3. Added inputs into the testcases
Change log:
1. Added Completed boolean
2. Added isDone, markDone, markUnDone and getStatusIcon methods
Change log:
1. Added size method
2. Added markDone method
Change log:
1. Added Checks for the command
Updated Duke to level-4
Copy link

@Raymond0212 Raymond0212 left a comment

Choose a reason for hiding this comment

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

Generally, the code quality is super good and the structure of this project is well-organized. In addition, there are tests for each class. I don't have much to say about it. Keep going!

Regards,
Yongyan.

@@ -0,0 +1,57 @@
package ultron.tasks;

public final class Todo extends Task {

Choose a reason for hiding this comment

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

Is there a need to declare Todo as a final class? I think there are still some possibilities to extend this class though we might not need it in our iP

Copy link
Author

Choose a reason for hiding this comment

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

When the extension comes, I can just remove the final after that and extend from there?
What I was thinking was that everything extends from the abstract class Task.
If there are other similar classes as todo we can all group them under todo or modify todo to accommodate other functions

Pattern.compile("^(.*) (/at) (.*)$");
/** Stores the date format regex.*/
private static final DateFormat DATE_FORMAT =
new SimpleDateFormat("dd-MM-yyyy HHmm");

Choose a reason for hiding this comment

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

Would it be simpler if you use LocalDateTime here?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, that might be true. Might edit in a future commit haha. Thanks for letting me know.


//Throw a Duke exception
throw new UltronException(command,
ExceptionType.INVALID_COMMAND);

Choose a reason for hiding this comment

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

Indentation for wrapped lines should be 8 spaces.

Copy link
Author

Choose a reason for hiding this comment

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

I see. Thank you!

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

5 participants