-
Notifications
You must be signed in to change notification settings - Fork 187
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
[Huang Maodian] iP #190
base: master
Are you sure you want to change the base?
[Huang Maodian] iP #190
Conversation
…comply with coding standard.
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.
Generally ok, but try to be more careful with magic number and string. Avoid long and complicated function/statement.
src/main/java/huan/main/Huan.java
Outdated
@@ -0,0 +1,142 @@ | |||
package huan.main; | |||
|
|||
import huan.task.*; |
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.
Avoid importing *, try to explicitly list out the import classes
src/main/java/huan/main/Huan.java
Outdated
} | ||
} | ||
} | ||
public static void readProcessCommand() { |
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.
Very long method, maybe try to abstract each command?
src/main/java/huan/main/Huan.java
Outdated
String firstWord = words[0]; | ||
String suffixWord; | ||
if(words.length > 1) { | ||
suffixWord = inputCommand.substring(words[0].length() + 1); |
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.
Multiple usage of Magic numbers, try to assign the number with a meaningful variable name for easy understand of what the number is for.
String[] words = nameWithDate.split(" "); | ||
/* | ||
state: | ||
0 means currently concatenating name |
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.
Can consider using ENUM instead of 0,1 for the state
import java.util.Objects; | ||
|
||
public class DeadlineTask extends Task{ | ||
private String ddlTime; |
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.
maybe can consider calling it deadlineTime, as ddlTime can be hard to understand what it means
} | ||
} | ||
} | ||
if(state != 2 || name.toString().isEmpty() || startTime.toString().isEmpty() || endTime.toString().isEmpty()) { |
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.
very long and complicated statement, can consider splitting it up
…ng deadline tasks before a specific time.
Add date time recognition in DeadlineTasks, and a command for filteri…
Add 'find' feature that allows filtering tasks by matching names
Add Javadoc Comments.
No description provided.