-
Notifications
You must be signed in to change notification settings - Fork 270
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
[Han Jiyao] iP #303
base: master
Are you sure you want to change the base?
[Han Jiyao] iP #303
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.
LGTM. If you're able to encapsulate the behaviour of all the actions of Duke then it would be better
src/main/java/Deadline.java
Outdated
@Override | ||
public String toString() { | ||
return "["+Type.D+"]" + super.toString() + " (by: " + by + ")"; | ||
} |
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.
Well done on the naming of variables, they are easily read!
src/main/java/Duke.java
Outdated
|
||
case "deadline": | ||
StringBuilder deadline = new StringBuilder(); | ||
StringBuilder deadlineBy = new StringBuilder(); |
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 encapsulate this functionality in another class if you want
src/main/java/Event.java
Outdated
this.at = at.trim(); | ||
} | ||
|
||
public Event(String task, boolean done) { |
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.
Easy to read!
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.
Overall good job 🙂!
Just a few points to mention:
- OOP practices can be implemented better
- Coding standard should be followed consistently
- Seems like you are a little bit behind schedule, looking forward to your GUI implementation!
} | ||
|
||
public void execute(TaskList tasks, Ui ui, Storage storage) { | ||
tasks.add(new Deadline(newDeadline.getTask(), newDeadline.getDate())); |
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 use getter method to keep fields in Deadline
class private. Good job 👍!
public void execute(TaskList tasks, Ui ui, Storage storage) { | ||
tasks.add(new Deadline(newDeadline.getTask(), newDeadline.getDate())); | ||
storage.saveTaskList(tasks); | ||
ui.showMessage("Got it. I've added this task: \n " |
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.
Extra blank spaces after the new line character is a little bit misleading, maybe can move these blank spaces into the next line.
tasks.add(new Deadline(newDeadline.getTask(), newDeadline.getDate())); | ||
storage.saveTaskList(tasks); | ||
ui.showMessage("Got it. I've added this task: \n " | ||
+ tasks.getByIndex(tasks.getSize() - 1) + "\n Now you have " + tasks.getSize() + " tasks in the list."); |
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.
Not sure if this line of code is too long for our coding standard, but generally try to break up a long line of code like this.
import task.TaskList; | ||
import ui.Ui; | ||
|
||
public class ErrorCommand extends Command { |
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 love your implementation of having an abstract class Command
and various other commands that extend it, however, personally I do not think an ErrorCommand
is considered as a Command
, because if you think about it, it is not like the user wants to input something intentionally that triggers this. The code works perfectly fine, it's just that why not throw exception directly when parsing the user input in the first place, instead of having recognized that the user input something invalid but still parse it to a Command
?
@Override public void execute(TaskList tasks, Ui ui, Storage storage) { | ||
ui.showExitMessage(); | ||
} | ||
|
||
@Override public boolean isExit() { | ||
return true; | ||
} |
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.
The @Override
tag should occupy its own line.
src/main/java/parser/Parser.java
Outdated
import command.ExitCommand; | ||
import command.MarkCommand; | ||
import command.ListCommand; | ||
import data.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.
Not sure if this line follows the import order in checkStyle.
src/main/java/storage/Storage.java
Outdated
} | ||
break; | ||
case "D": | ||
Deadline newDeadline = new Deadline(input.get(2).trim(), LocalDate.parse(input.get(3).trim(), formatter)); |
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.
Sufficient comment here can probably improve the readability by a lot.
src/main/java/task/Deadline.java
Outdated
import java.time.format.DateTimeFormatter; | ||
|
||
public class Deadline extends Task { | ||
protected LocalDate by; |
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.
private
should be a better accesser here than protected
because there are no other classes extending from Deadline, therefore, the only object that will access this field is of type Deadline
. Declaring this field as final
should also be a good practice because the time should not be changed after new()
an object.
src/main/java/task/Deadline.java
Outdated
|
||
@Override | ||
public String saveData() { | ||
int done = super.done? 1 : 0; |
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.
Declaring this as boolean isDone
might be better.
src/main/java/task/Task.java
Outdated
public Task mark() { | ||
return new Task(task, true); | ||
} | ||
|
||
public Task unmark() { | ||
return new Task(task, false); | ||
} |
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.
Not too sure about the rationale behind the need to new()
an object, (same for Deadline
Event
and Todo
), why not just simply set the done
field as true or false, all the children of Task
share the same behaviour and they can just access mark()
and unmark()
straight away.
There is a method that previous used as textUI to convert cli command into console output. This method is never used as GUI has implemented. Removing all these unused code improves the code quality. As a step towards such unification, let's remove those unused code blocks in their respective classes. Doing so will make the code easier to understand.
Duke: getResponse() add assertion Parser: parse() add assertion Storage: load() add assertion Ui: showMessage() add assertion MainWindow: add DukeException Add assertion failure indicates possible bug Use assert feature aim to document important assumptions that should hold at various points in the code. Let's update the methods with assertions. Using assertion is preferable.
Improve code quality
Add assertion
There is a name error from prepareAddTodo() to prepareAddToDo(). Fixing this name error will resolve the application cannot run. Let's update the method name inside the Parse class.
Mark an item as a high normal low priority
Branch c priority sort
Branch c priority sort
Duke Chat
DukePro frees your mind of having to remember things you need to do. It's,
FASTSUPER FAST to useAll you need to do is,
And it is FREE!
Features:
If you Java programmer, you can use it to practice Java too. Here's the
main
method: