-
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
[jellywaiyan] iP #538
base: master
Are you sure you want to change the base?
[jellywaiyan] iP #538
Conversation
# Conflicts: # src/main/java/Jelly/main/Storage.java # src/main/java/Jelly/main/TaskList.java # src/main/java/Jelly/main/Ui.java # src/main/java/Jelly/task/Deadline.java # src/main/java/Jelly/task/Event.java # src/main/java/Jelly/task/Todo.java
# Conflicts: # src/main/java/Jelly/main/Jelly.java # src/main/java/Jelly/main/Ui.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.
Looks good
Quite comprehensive documentation
Well designed
src/main/java/Jelly/task/Event.java
Outdated
@@ -54,6 +58,7 @@ public String toString() { | |||
} | |||
return "[E]" + super.toString() + "(from: " + fromWhen + " to: " + toWhen + ")"; | |||
} | |||
|
|||
@Override | |||
public String writeToFile() { |
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.
Could be better named.
I do not expect"writeToFile" to be a method that returns the String to be written
src/main/java/Jelly/main/Jelly.java
Outdated
/** | ||
* The main initialiser for the Jelly Chat bot. | ||
* | ||
* @param args |
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 do not think "@param args" help much
And I believe that you do not need to document every method as well.
src/main/java/Jelly/main/Ui.java
Outdated
public Ui() { | ||
sc = new Scanner(System.in); | ||
} | ||
|
||
/** | ||
* Prints out a welcome message when the Chat Bot is booted up. | ||
*/ | ||
public void startUpMessage() { |
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.
sounds like a noun
src/main/java/Jelly/main/Ui.java
Outdated
* @param addedTask The task that was added. | ||
* @param noOfTasks The total number of tasks in the list after adding. | ||
*/ | ||
public void addedTaskMessage(Task addedTask, int noOfTasks) { |
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.
noun
src/main/java/Jelly/main/Ui.java
Outdated
|
||
/** | ||
* Displays a final message to the user before the Chat Bot shuts down. | ||
*/ | ||
public void byeMessage() { |
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.
noun
src/main/java/Jelly/task/Task.java
Outdated
/** | ||
* Should not happen, as tasks should be either one of these: todo, deadline or event. | ||
* @return Error message. | ||
*/ | ||
public String writeToFile() { |
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.
if this should not happen, why not throw exception when called?
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 great! No major coding standard issues, just some minor issues with how you name some variables like the other reviewer have mentioned
private String description; | ||
|
||
private String fromWhen; | ||
|
||
private String toWhen; |
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 can be grouped together
import Jelly.task.Task; | ||
import Jelly.task.Todo; | ||
import org.junit.jupiter.api.Test; | ||
import static org.junit.jupiter.api.Assertions.assertEquals; | ||
import static org.junit.jupiter.api.Assertions.assertTrue; | ||
import static org.junit.jupiter.api.Assertions.assertFalse; |
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 separate out the imports?
|
||
public class StorageTest { | ||
@Test | ||
public void startUpTest() throws IOException, JellyException { |
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.
Test methods name can utilize another convention for naming them: featureUnderTest_testScenario_expectedBehavior()
Delete command: Save to file after deleting Mark command: Save to file after marking a task as done Unmark command: Save to file after marking a task as not done Previously, these commands did not save to file, which caused the file to be outdated. Let's, *add a saveToFile method after completing the command
Improve code quality by making methods more efficient and readable.
Assert feature allows the documentation of important assumptions that should hold at various points in the code. Let's, *add assertions in Task, Parser,DeleteCommand, MarkCommand, UnmarkCommand.
Current implementation does not use assertions.
Jelly
Jelly frees your mind of having to remember things you need to do. It's,
FASTMAYBE FAST to use :oAll you need to do is,
And it is FREE!
Features:
If you are a java programmer, you can use it to practice Java too. Here's the
main
method: