Skip to content
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

Chia Chu You iP #130

Open
wants to merge 63 commits into
base: master
Choose a base branch
from
Open

Conversation

chuyouchia
Copy link

No description provided.

…ith errors such as incorrect inputs entered by the user.
…rrentStatus method call. Level-5: Teach Duke to deal with errors such as incorrect inputs entered by the user.
Original code contained UI and logic in the main method.

As a result, if there are any changes to be made to a specific scenario, problems are not isolated into individual methods or cases.

Simplified the driver method to be solely responsible for console input and dependent on other static methods.

By separating the responsibility into smaller chunks, it will improve debugging process if necessary.

Java Doc comments have been added to describe individual methods as well.
Original code was treating all date time formats simply as strings.

Add the function for the tasks to have stored date time. Command "list-due" added for filtering options on deadline and commitments.
…ndler.

Save the task list to hard disk so that past tasks recorded would not be lost when the program terminates.
# Conflicts:
#	src/main/java/com/jacob/Duke/Deadline.java
#	src/main/java/com/jacob/Duke/Duke.java
#	src/main/java/com/jacob/Duke/Event.java
#	src/main/java/com/jacob/Duke/Task.java
…ace into Packages

Original code had repeated UI elements and combined Commands with Parser, which resulted in unnecessary repeated code between commands.

Extracted the following classes:
- Ui: deals with interactions with the user
- Renamed FileEditor to Storage: deals with loading tasks from the file and saving tasks in the file
- Parser: deals with making sense of the user command
- TaskList: contains the task list e.g., it has operations to add/delete tasks in the list
- Command: that implements a Command Interface with Execute and isBye methods.

Extracting the class allows for method reusing and modularity as codebase grows.
JUnit enables automated testing for Test Driven Development.

Note:
-     implementation 'org.junit.jupiter:junit-jupiter-api:5.5.1'  is required to make sure that the import for @test annotation does not fail
-     Set the System.out to a byteArrayOutput Stream so that it is possible to capture console output for testing purposes.
# Conflicts:
#	src/main/java/com/jacob/duke/DukeException.java
#	src/main/java/com/jacob/duke/TaskList.java
#	src/main/java/com/jacob/duke/command/ByeCommand.java
#	src/main/java/com/jacob/duke/command/DeadlineCommand.java
#	src/main/java/com/jacob/duke/command/DeleteCommand.java
#	src/main/java/com/jacob/duke/command/DoneCommand.java
#	src/main/java/com/jacob/duke/command/EventCommand.java
#	src/main/java/com/jacob/duke/command/PrintFilteredListCommand.java
#	src/main/java/com/jacob/duke/command/PrintListCommand.java
#	src/main/java/com/jacob/duke/task/Todo.java
Give users a way to find a task by searching for a keyword.

# Conflicts:
#	src/main/java/com/jacob/duke/Ui.java
#	src/main/java/com/jacob/duke/command/PrintFilteredListDateTimeCommand.java
Copy link

@yuxuanxc yuxuanxc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the main issue is with the javadoc comments, maybe you will need to spend some time fixing your current comments to adhere to the coding standard guidelines. Remember to put a full stop behind every summary, param, return etc and leave an empty line after the summary before you start writing the parameters in the future! Sorry for spamming a lot of comments regarding extra spaces, I just thought that removing them would make your code look more consistent and organized. Other than that, the naming and layout all seems fine. Good job! :D


/**
* Constructor for Duke
* @param filePath contains file where task list is saved
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be a punctuation (full-stop) behind each parameter description.
Remember to put an empty line between description and parameter section.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, thank you for pointing that out!

private Ui ui;

/**
* Constructor for Duke
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to include a full-stop at the back of the summary to indicate the end of the sentence, so that Javadoc will automatically place it in the method summary table(of your API)?

@@ -0,0 +1,58 @@
package main.java.com.jacob.duke;

import main.java.com.jacob.duke.command.*;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we still need to import every single class specifically instead of using a wildcard(*), even if all of the classes in that package is being used. I went to google about this a little but there are conflicting answers as to whether wildcards should be used or not, so maybe you can clarify it with your TA/the profs?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually did try that, but it seems like my IntelliJ automatically refactors the code to the * type import. Are you facing the same problems?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found this online! (source: https://www.jetbrains.com/help/idea/creating-and-optimizing-imports.html#exclude-from-auto-import)
Disable wildcard imports to always import single classes:

  1. In the Settings/Preferences dialog Ctrl+Alt+S, select Code Style | Java | Imports.
  2. Make sure that the Use single class import option is enabled.
  3. In the Class count to use import with ‘’ and Names count to use static import with ‘’ fields, specify values that definitely exceed the number of classes in a package and the number of names in a class (for example, 999).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, that works 👍

import main.java.com.jacob.duke.task.Task;
import main.java.com.jacob.duke.task.Todo;


Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a few extra spaces here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, thank you for pointing that out!

java.nio.file.Path directoryPath = java.nio.file.Paths.get(current, parent);
boolean directoryExists = java.nio.file.Files.exists(directoryPath);


Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's one extra space here, not sure if it is a violation but you might want to delete it so that your code will look more consistent throughout? There is one extra space at line 65 too

@@ -0,0 +1,31 @@
package main.java.com.jacob.duke.command;
import java.util.List;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to add a space above!

@@ -0,0 +1,58 @@
package main.java.com.jacob.duke.command;
import java.util.List;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need space above this line.

public DeadlineCommand(String fullCommand) {
this.inputCommand = fullCommand;
}
/**
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need space above this line?

import main.java.com.jacob.duke.TaskList;
import main.java.com.jacob.duke.Ui;

import main.java.com.jacob.duke.task.Task;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra spaces in lines 8, 11 and 12

import main.java.com.jacob.duke.Ui;
import main.java.com.jacob.duke.task.Task;


Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why got so many spaces haha. This is not a violation so I don't want to keep spamming the same comment.

Just in case if you want to fix the extra spaces for the sake of aesthetics, here are some which I found in other files which I didn't comment:
TodoCommand.java - line 12
Event.java - line 3
UiTest.java - line 14

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for pointing that out! Will refactor accordingly in my next release 👍

Refactor UI and UI testing to support string validation
Copy link

@hopinxian hopinxian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, I like the assertions used to support your program. I think that is something I ought to learn. There are still some room for improvements. If you are using Intellij, I would suggest using the "Reformat code" function under the code tab. This would help you with the random white spaces that appear.

data/duke.txt Outdated
Comment on lines 1 to 4
D,1,return book,2019-11-13 1800
T,0,funny
T,0,nani what is happening to me
E,0,omomomo,2019-11-15 1900

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May I suggest adding more line inputs with respect to the increasing levels? I find that maintaining this text assists greatly with testing.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup this make sense. I actually have separate test files and duke.txt is really just a dummy text file that I ignored after the start.

@@ -0,0 +1,63 @@
package main.java.com.jacob.duke;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May I suggest putting the Parser and Storage class within a IO package?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, thanks for the suggestion!

Comment on lines 142 to 143
* Replaces a specific line of text in the string buffer read
* @param lineToEdit text to be replaced

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe within the coding standard, it is stated there should be a line break between the description and parameter block. Perhaps you check to confirm? This goes for other places within this project as well.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup thanks, have rectified in my latest release.

Comment on lines 33 to 43
List<Task> taskList = tasks.getTaskList();
int breakpoint = inputCommand.indexOf("/") - 1;
if (inputCommand.length() <= "deadline ".length()) {
throw new DukeException("☹ OOPS!!! The description of a Deadline cannot be empty.");
} else if (breakpoint == -2) {
throw new DukeException("Hey a deadline cannot have no actual date!!");
}
String description = inputCommand.substring("deadline".length() + 1, breakpoint);
String dateTime = inputCommand.substring(breakpoint + 5);
Task theDeadline = new Deadline(description, dateTime);
taskList.add(theDeadline);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This block feels quite lengthy. Perhaps it would be better to add some line breaks for readability.

import main.java.com.jacob.duke.command.PrintListCommand;
import main.java.com.jacob.duke.command.TodoCommand;

public class Parser {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe there is quite some functions within other classes that would be better encapsulated within this class. Examples are detecting for invalid indexes, incomplete commands and so forth. I would suggest that Parser also have additional capabilities to identify invalid commands. I believe this would fall under the role of parsing the user input.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup agreed, thanks for the insight! I will probably refactor instead of using line breaks to improve readability

Place parser and storage into io package for better readability and
categorization
It is now called the fireside chat with the Mamba.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants