-
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
[Guo Yulong] iP #166
base: master
Are you sure you want to change the base?
[Guo Yulong] iP #166
Conversation
# Conflicts: # src/main/java/Deadline.java # src/main/java/Event.java
# Conflicts: # src/main/java/duke/command/CommandDelete.java # src/main/java/duke/command/CommandTask.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, I found your code neat and have followed the coding standard. I noticed that you have not added JavaDoc comments for your classes. Consider including them in.
|
||
import java.time.LocalDate; | ||
import java.time.format.DateTimeFormatter; | ||
|
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 include JavaDoc comments for each class?
src/test/java/duke/ParserTest.java
Outdated
|
||
assertEquals(true, command.isExit()); | ||
} | ||
} |
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.
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 neat and readable code that follows Java coding convention well. My only suggestions are to consider trailing whitespace during parsing and to rename the test methods to be more in line with convention! Great job Yulong!
@@ -72,7 +76,9 @@ public static Command parse(String fullCommand) { | |||
int indexFrom = fullCommand.indexOf("/from"); | |||
int indexTo = fullCommand.indexOf("/to"); | |||
if (indexFrom != -1 && indexTo != -1) { | |||
t = new Event(fullCommand.substring(6, indexFrom), fullCommand.substring(indexFrom + 6, indexTo - 1), fullCommand.substring(indexTo + 4)); | |||
t = new Event(fullCommand.substring(6, indexFrom), |
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.
Neat indexing that follows coding standard. Well done!
src/test/java/duke/ParserTest.java
Outdated
|
||
public class ParserTest { | ||
@Test | ||
public void dummyTest(){ | ||
public void dummyTest() { |
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 using another name for test method would be more appropriate. Perhaps using the test name convention whatIsBeingTested_descriptionOfTestInputs_expectedOutcome
@@ -25,7 +28,7 @@ public String getDesc() { | |||
* Sets the status of the task. | |||
* @param isDone final status of the task. | |||
*/ | |||
public void setStatus(Boolean isDone){ | |||
public void setStatus(Boolean isDone) { |
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 combine mark and unmark method into a single setStatus method!
src/main/java/duke/Parser.java
Outdated
@@ -72,7 +76,9 @@ public static Command parse(String fullCommand) { | |||
int indexFrom = fullCommand.indexOf("/from"); | |||
int indexTo = fullCommand.indexOf("/to"); | |||
if (indexFrom != -1 && indexTo != -1) { | |||
t = new Event(fullCommand.substring(6, indexFrom), fullCommand.substring(indexFrom + 6, indexTo - 1), fullCommand.substring(indexTo + 4)); |
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.
Good parsing practices but I also recommend using method trim()
to remove the leading and trailing whitespace in the task description!
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.
Great job on the code overall!
private Storage storage; | ||
private TaskList tasks; | ||
private 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.
I like how you stored the information!
public static Command parse(String fullCommand) { | ||
|
||
if (fullCommand.equalsIgnoreCase("bye")) { | ||
return new CommandBye(fullCommand); |
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.
Might want to consider using a switch case to make code more readable
System.out.println("Now you have " + len + " in the list."); | ||
|
||
storage.writeArray(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.
Neat code even printing complex set of text
System.out.println("Here are the matching tasks in your list:\n"); | ||
for (int i = 0; i < taskList.getLength(); i++) { | ||
if (taskList.getTask(i).getDesc().contains(word)) { | ||
System.out.printf("%d.%s\n", i + 1, taskList.getTask(i).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.
Might be more readable to split these functions up instead of doing it all in one line
Added assertions to check that input line should be non-empty as this can cause exceptions. Also checked if the requested index was within the length of Tasklist before marking.
Refactored code to make it more readable using switch statements and more intermediate variables.
Add A-Assertions
# Conflicts: # src/main/java/duke/task/Task.java
Add A-CodeQuality
Ability to undo to add more flexiblity for the user. Creating a temporary store to the arraylist for the previous command. Introduce inverse commands which undo the effect of the previous command. To reduce adding more classes and utilise the existing abstraction to achieve the feature.
This reverts commit 8501c16.
This reverts commit d294575.
Fixed bugs with undo feature.
Change import statements
Included examples of features and screenshot of start UI.
Update import order to align with the guidelines.
DukePro
DukePro frees your mind of having to remember things you need to do. It's,
All 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: