-
Notifications
You must be signed in to change notification settings - Fork 461
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
[Bryan Lim Jing Xiang] iP #484
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall very well written to comply with the Java Coding standard. Great use of access modifiers and naming was very clear. Great job! 😀
*/ | ||
public abstract class Task { | ||
private final String taskItem; | ||
private boolean isMarked; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good naming of boolean variables, makes it easier to follow the code 👍
* @return DeadlineDateTime object that stores the Date and Time | ||
* @throws DukeException If the storedDateTime cannot be parsed | ||
*/ | ||
public static DeadlineDateTime parseDateFromStorage(String storedDateTime) throws DukeException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great naming of methods, clear usage of verbs 😄
src/main/java/util/Ui.java
Outdated
public static String formatLinesIntoParagraph(String... lines) { | ||
String res = ""; | ||
for (String line : lines) { | ||
res += formatLine(line); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you're concatenating Strings here, maybe you could consider using the StringBuilder object to optimise your code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work! I couldn't really find any coding standard violations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could add more unit test for more of the classes.
The packages are done neatly with the related classes placed in the same folder (package). Follows good OO practices, good naming of variables and well code is well documented.
Date format for event datetime is `yyyy-mm-dd hh:mm:ss hh:mm:ss`, not `yyyy-mm hh:mm:ss hh:mm:ss hh:mm:ss`. This has been rectified in this commit.
Added validations after validating arguments/format for Todo arguments, EventDateTime, and DeadlineDateTime.
There were a few cases where String concatenation was used within a loop instead of StringBuilder, which is not ideal due to performance reasons. This was fixed in this commit.
Add Assertions
Improve code quality
Duke
Duke frees your mind of having to remember things you need to do. It's,
FASTSUPER FAST to useAll you need to do is,
And it is FREE!
Features:
If you Java programmer, you can use it to practice Java too. Here's the
main
method:Future improvements
Work in progress: