-
Notifications
You must be signed in to change notification settings - Fork 437
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
[Xu ZhiZhi] iP #336
base: master
Are you sure you want to change the base?
[Xu ZhiZhi] iP #336
Conversation
bd6fbeb
to
74f6376
Compare
74f6376
to
b71a498
Compare
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.
You have done such a wonderful job of following the coding standards. To be honest, I had a hard time finding any errors or mistakes in your code. Keep up the good work 👍
} else { | ||
throw new DukeException(DukeException.IGNORE); | ||
} | ||
} else if (task.length > 0 && task[0].equals("todo")){ |
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.
Shouldn't the 'if condition' and the 'following {' be separated by a single whitespace?
src/main/java/TaskList/Storage.java
Outdated
import java.io.IOException; | ||
import java.util.ArrayList; | ||
|
||
import Tasks.*; |
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.
Perhaps the relevant Task libraries could be listed and imported explicitly instead of using a wildcard import?
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 code quality and excellent adherence to coding conventions overall!
import TaskList.TaskList; | ||
|
||
public class FindCommand extends Command { | ||
private String searchText; |
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.
Perhaps userInput
would be a more suitable variable name than searchText
?
return (completed ? "[✓]" : "[✗]") + " " + name; | ||
} | ||
|
||
public enum Type { |
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.
I like your suitable use of enum here!
this.type = type; | ||
} | ||
|
||
public enum CommandType { |
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.
Nice use of enum here!
src/main/java/DukeComponent/Ui.java
Outdated
return s; | ||
} | ||
|
||
public static String welcomeMessage() { |
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.
Should the method be named more accurately with a verb, perhaps getWelcomeMessage()
?
src/main/java/DukeComponent/Ui.java
Outdated
return print("Try again! Date format should be yyyy-mm-dd."); | ||
} | ||
|
||
public static String exitMessage() { |
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.
Should the method be named more accurately with a verb, perhaps getExitMessage()
?
No description provided.