-
Notifications
You must be signed in to change notification settings - Fork 482
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
[lordidiot] iP #527
base: master
Are you sure you want to change the base?
[lordidiot] iP #527
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.
Looks good 👍
Just a few nitpicks here and there
private static boolean handleInput(Parser parser, TaskList taskList, Ui ui) throws DukeException { | ||
Task task; | ||
switch (parser.getCommand()) { | ||
case "bye": |
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 it would be better to consider the case that the user might input the correct word but a different capitalisation? For eg. instead of bye, the user inputs Bye or BYE.
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.
Definitely, will work on that.
src/main/java/duke/Duke.java
Outdated
ui.exit(); | ||
} | ||
|
||
private static boolean handleInput(Parser parser, TaskList taskList, Ui ui) 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.
I like the function in that it is succinct and it is very readable. However, I think you might want to consider refactoring the code and extracting this function into the Parser class as that class deals with making sense of the user commands.
src/main/java/duke/Ui.java
Outdated
* Initialise UI. Prints banner. | ||
*/ | ||
public void init() { | ||
System.out.println("____________________________________________________________"); |
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.
Small thing but perhaps it would be better to turn the underline into a constant so it can be easily reused elsewhere
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.
Thank you! That makes sense, will do so.
src/main/java/duke/TaskStorage.java
Outdated
return new ArrayList<>(); | ||
} | ||
|
||
ArrayList<Task> tasks = new ArrayList<>(); |
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 how you used tasks as the variable name for the Java Collections Object
package duke.mocks; | ||
|
||
import duke.Task; | ||
public class TaskMock extends 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.
I really like the usage of stubs when performing your unit testing. Really shows you went the extra mile!
src/main/java/duke/TaskStorage.java
Outdated
* @throws DukeException Any errors encountered while storing | ||
*/ | ||
public void storeTasks(List<Task> tasks) throws DukeException { | ||
String serialized = tasks.stream() |
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 cool usage of streams 👍
Minimise code reptition by replacing the dotted line string constants with a single static reference to a class constant DOTTED_LINE.
Add assertions
Refactor to improve code quality
A heading
🤓
a hyper link
inline code?
some text formatting: bold, italic,
strikethroughetc.