-
Notifications
You must be signed in to change notification settings - Fork 364
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
[HAO ZEYU] iP #281
base: master
Are you sure you want to change the base?
[HAO ZEYU] iP #281
Conversation
# Conflicts: # src/main/java/duke/parser/Parser.java # src/main/java/duke/ui/Ui.java
# Conflicts: # src/main/java/duke/command/Command.java # src/main/java/duke/command/MarkDoneCommand.java # src/main/java/duke/command/MarkUndoneCommand.java # src/main/java/duke/command/ShowListCommand.java # src/main/java/duke/storage/Storage.java # src/main/java/duke/tasks/Deadline.java # src/main/java/duke/tasks/Event.java # src/main/java/duke/tasks/Task.java
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, great job! 👍 Some minor issues I found
- Missing comments for classes (I may have missed a class or two) but I think it will be helpful to include briefly what the class function is.
- Perhaps not committing your txt file to github.
Other than that, the code looks good!
Please let me know if you have any queries regarding my comments.
data/duke.txt
Outdated
@@ -0,0 +1,4 @@ | |||
E | 1 | say hi | from 2 to 5 |
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.
Just a suggestion. You may want to consider adding duke.txt into your .gitignore file to avoid the constant file change when committing changes.
src/main/java/duke/Duke.java
Outdated
|
||
|
||
|
||
|
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 you can consider adding a comment for the Duke class briefly explaining what the Duke class do.
@@ -0,0 +1,5 @@ | |||
package duke; | |||
|
|||
public enum Commands { |
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 have an enum to manage the different commands.
|
||
|
||
|
||
|
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.
Similar comments as above regarding the comment for the class.
import duke.ui.Ui; | ||
|
||
|
||
|
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.
Similar comments as above regarding the comment on class (for all classes under command folder)
@@ -0,0 +1,99 @@ | |||
package duke.tasks; | |||
|
|||
public class 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.
Similar comments as above regarding comments on class.
assertEquals("[E][ ] finish homework (from: 2pm to: 4pm)", new Event("finish homework", "from 2pm to 4pm").toString()); | ||
} | ||
|
||
} |
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 think you can include a new line here. I don't think its compulsory but github warns you about it. I found a reason online in this link here if you're interested.
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, I found the 'Naming' aspect of Code Quality to be well-satisfied. Just a few minor things to 'fix'. Great job! 😄
src/main/java/duke/Duke.java
Outdated
ui = new Ui(); | ||
storage = new Storage(filePath); | ||
try { | ||
tasks = new TaskList(storage.load()); | ||
} catch (DirectoryNotFoundException e) { | ||
ui.showError(e.getMessage()); | ||
tasks = new TaskList(); | ||
} catch (FileNotFoundException e) { | ||
ui.showError(e.getMessage()); | ||
tasks = new TaskList(); |
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 naming used here is very straightforward and easy to understand 👍
try { | ||
Scanner s = new Scanner(file); // create a Scanner using the File as the source | ||
while (s.hasNext()) { | ||
String cur = s.nextLine(); |
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 could have avoided the short form and have used a slightly more descriptive variable name (e.g. currentLine
)?
I also noticed similar instances in several other places as well...
* @return The task deleted | ||
*/ | ||
public Task delete(int index) { | ||
Task removed = taskList.remove(index); |
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 feel that it might be better to use the name removedTask
instead :)
I would sat the same about instances in several other places as well, (e.g. markedTask
, unmarkedTask
, etc.)
* @param storage The storage object | ||
*/ | ||
public void execute(TaskList list, Ui ui, Storage storage) { | ||
ArrayList<Integer> found = 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.
Maybe a more descriptive name like tasksFound
?
A-Assertions
Currently some imports and functions are redundant and some places need more comments for other developers to understand Let's remove the redundant code and add more comments to make the code clearer and more succint Other developers will not msiunderstand the code and they will be able to know why some parsing is done in that way
Code Quality: refactor the code to improve the code quality
If a user makes a mistake he or she cannot undo it Allowing user to undo give them the chance to recover from their mistake Let's enable undo options for delete task, mark a task as done, mark a task as undone, and add a task Undoing an undo is not allowed because this user will be unlikely to do so, and undoing show task and find task commands are trivial so they cannot be undone as well.
Updated the user guide.
Update user guide
Uploaded Product Screenshot
Uploaded Ui.png
Duke Pro
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: