[Jin Ming] iP #269
[Jin Ming] iP #269
Conversation
| * file and writing the updated tasks into the new storage file. | ||
| * | ||
| * @param tasks List of updated tasks. | ||
| */ |
There was a problem hiding this comment.
Should this method be called updateFile instead?
"update" alone may be too vague and can be confused with updating the class' state or updating individual task data
| * @param userCommand User's command keyword (first word in the user's input). | ||
| * @param key Supported command key. | ||
| * @return Whether user's command keyword is supported. | ||
| */ |
There was a problem hiding this comment.
Should this method just be called "equals"?
This method already belongs to the CommandKey enum, so it would be called as CommandKey.equalsCommandKey, which might be redundant.
Just having "equals" would also match the equals method for other data types like Strings
src/main/java/duke/ui/Ui.java
Outdated
| static String ADD_TASK_LINE = ">>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>"; | ||
| static String DONE_TASK_LINE = "~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~"; | ||
| static String LIST_TASK_LINE = "________________________________________________________"; | ||
| static String BYE_LINE = "========================================================"; |
There was a problem hiding this comment.
Would it be better to name these as "LINE_XXX_XXX" instead?
Should these variables be final?
They appear to be constants and prefixing them with the same word will make them appear together when sorted alphabetically (see coding standard document)
src/main/java/duke/task/Task.java
Outdated
| */ | ||
| public abstract class Task { | ||
| protected String task; | ||
| protected boolean done; |
| } | ||
|
|
||
| /** | ||
| * Tells Duke to keep on keeping on. |
| + this.date.format(DateTimeFormatter.ofLocalizedDate(FormatStyle.MEDIUM)) | ||
| + " " + this.time.format(DateTimeFormatter.ofLocalizedTime(FormatStyle.SHORT)) |
There was a problem hiding this comment.
Should you remove the this keyword? In the Java Coding Standard documentation called "Avoid unnecessary use of this with fields.", it mentions how the the this keyword should only be used when a field is shadowed by a method or constructor parameter.
| } | ||
|
|
||
| /** | ||
| * Tells Duke to keep on keeping on. |
There was a problem hiding this comment.
Comment could be clearer, not sure what it is trying to help with.
| * @param task | ||
| * @return true. | ||
| */ | ||
| public boolean add(Task task) { |
There was a problem hiding this comment.
Maybe this can be renamed as users might not be able to tell that it is a boolean on first look, maybe taskIsAdded?
|
Great job with the current progress, not much comments to give since your code is neat and have been commented out well for users. Doing great :) |
Add assertions A-Assertions
Improve code quality A-CodeQuality
Implement BCD Feature: B-Reminders
No description provided.