[Nicholas Nge] iP #257
[Nicholas Nge] iP #257
Conversation
# Conflicts: # src/main/java/Deadline.java # src/main/java/Event.java
| } | ||
|
|
||
| /** Creates a new Deadline | ||
| * @param ui helper to interact with user |
There was a problem hiding this comment.
Maybe there should be an empty line between the method description and parameter section
|
|
||
| /** Creates a new Event | ||
| * @param ui helper to interact with user | ||
| * @param tasks we add the new created event here |
There was a problem hiding this comment.
Perhaps could add punctuation behind each parameter description
| ui.showFormatException("FindCommand"); | ||
| } | ||
| return new EmptyCommand(); | ||
| } |
There was a problem hiding this comment.
maybe can stick with } else { indentation, comment can be moved up
There was a problem hiding this comment.
Overall, the code is clean and easy to understand.
Just a few things to take note of:
- class field should be after the class header.
- remember to add a space before "{" in class header.
- For the package name, can consider put it all under one big package. eg duke.command, duke.oracle.
| Task t = tasks.get(this.taskIndex); | ||
| tasks.remove(this.taskIndex); |
There was a problem hiding this comment.
Task t = tasks.remove(this.taskIndex)
I think can make this simpler by combining both statements.
| import oracle.TaskList; | ||
| import oracle.Ui; | ||
|
|
||
| public class EmptyCommand implements Command{ |
There was a problem hiding this comment.
Can have a space after between Command and {
| /** | ||
| * @param ui helper to print the new deleted task | ||
| * @param tasks we call delete on the indicated task in this TaskList | ||
| * @return true |
There was a problem hiding this comment.
Can explain why the method always returns true. eg So that duke can continue accepting commands. (I understand this after reading the ExitCommand)
| public boolean execute(Ui ui, TaskList tasks) { | ||
| ArrayList<Task> results = new ArrayList<>(); | ||
| for (Task t : tasks.getTasks()) { | ||
| if (t.toString().contains(this.keyword)) { |
There was a problem hiding this comment.
I think can give a variable to t.toString() to explain whether this is the task description or the string representation of the task.
src/main/java/entry/Deadline.java
Outdated
| return res; | ||
| } | ||
|
|
||
| LocalDateTime deadline; |
There was a problem hiding this comment.
This field should be right after header?
There was a problem hiding this comment.
yup, thanks for pointing it out
src/main/java/entry/Task.java
Outdated
| @@ -0,0 +1,54 @@ | |||
| package entry; | |||
|
|
|||
| public class Task { | |||
There was a problem hiding this comment.
Add a header comment to describe the use of this task.
I think can declare Task as an abstract class since there is no Task instance.
src/main/java/oracle/Parser.java
Outdated
| return new EmptyCommand(); | ||
| } else if (split[0].equals("find")) { | ||
| try { | ||
| String kw = split[1]; |
There was a problem hiding this comment.
Can give a more descriptive name, ie what does kw stands for?
| * @param input this is the raw String given by the user | ||
| * @return a Command | ||
| */ | ||
| public Command parse(String input) { |
There was a problem hiding this comment.
Is this method a bit too lengthy, can consider extract out the common part of the code.
| FileWriter myWriter = new FileWriter(filePath); | ||
| for (Task task : tasks){ | ||
| myWriter.write(task.toStorage()+ '\n'); | ||
| } |
There was a problem hiding this comment.
sorry, how is it off? i cant see it
| String[] sorted = s.split("\u001E"); | ||
| Boolean isDone = sorted[1].equals("T"); | ||
| try{ | ||
| switch (sorted[0]) { |
There was a problem hiding this comment.
Add a default case as specific in textbook.
A-Assertions
…eQuality � Conflicts: � src/main/java/entry/Task.java � src/main/java/oracle/Parser.java � src/main/java/oracle/TaskList.java
A code quality
A-BetterGui
No description provided.