[Javier Lim] iP #446
[Javier Lim] iP #446
Conversation
Incomplete note on some methods were removed
* 'master' of https://github.com/ijavierja/ip: Remove incorrect notes
Save method puts the list of tasks into a text file. This allows the client to access tasks even if Duke has been closed. The path of the text file can be found in the Duke.java file as a global variable at the top of the file.
…to allow only a date time description Level 8. Date and Times. The Task class was modified to include a new field of type LocalDateTime. This is to keep the Time and Date for the tasks. Deadline method was also modified to only allow for a date and time description.
# Conflicts: # src/main/java/Duke.java
ui class deals with interaction with the user. Ui class takes commands from the user as well as print output to user.
TaskList stores an ArrayList<Task> and deals with all changes to this list. Additionally, the class deals with all outputs needed from this arraytist
Storage class deals with loading tasks from the file and saving tasks in the file DukeException is thrown when no file is present with the name represented by filePath
Command is an interface with two abstract methods. Parser deals with making sense of the user command. A Command instance is implemented using Parser's static parse method.
…hod, execute, output type to String from void Test classes uses JUnit for testing of the classes' methods.
Only header comments for classes have been added
List all task that contains the phrase
# Conflicts: # src/main/java/Parser.java # src/main/java/TaskType.java # src/main/java/Ui.java
There was a problem hiding this comment.
Overall, I found your code easy to read in most of the parts except that some methods could be extracted into separate classes to further improve the readability and prevent the method being too complicated, if make sense. I noticed that there are several violations of the coding standard such as the completeness of Javadoc comments and indentation.
src/main/java/Duke.java
Outdated
|
|
||
| /** | ||
| * Constructor which takes in file path of the storage file. | ||
| * @param filePath |
There was a problem hiding this comment.
Perhaps add a description to the parameter name in the Javadoc comments to make it clearer?
src/main/java/Parser.java
Outdated
| * Returns the first word in the string indicated by the first blank space. | ||
| * If there are no blank space, the entire string is returned. | ||
| * | ||
| * @param string |
There was a problem hiding this comment.
Perhaps add a description for the return value for Javadoc comments to make it more complete? I noticed this issue in several other places as well.
src/main/java/Parser.java
Outdated
|
|
||
| public class Parser { | ||
| public static Command parse(String fullCommand) throws DukeException { | ||
| if(fullCommand.equals("list")) { |
There was a problem hiding this comment.
I like the way the if-else conditions are constructed. They are really clear!:) 👍
src/main/java/Parser.java
Outdated
| public static Command list() { | ||
| return new Command() { | ||
| @Override | ||
| public String execute(TaskList taskList, Ui ui, Storage storage) { |
There was a problem hiding this comment.
Should this part be extracted out? Perhaps consider creating multiple command class to handle different commands in order to improve the readability of following command method? :)
src/main/java/TaskList.java
Outdated
| * TaskList contains an ArrayList of Task. | ||
| * It serves as a barrier to directly using the ArrayList class so as to limit the functions available to the client. | ||
| */ | ||
| public class TaskList { |
There was a problem hiding this comment.
missing javadocs for methods in this classs
src/main/java/Task.java
Outdated
| } | ||
| public String getDoneString() { | ||
| String string; | ||
| if(isDone){ |
There was a problem hiding this comment.
missing spaces between parentheses and brace
src/main/java/Task.java
Outdated
| */ | ||
|
|
||
| public class Task { | ||
| duke.TaskType taskType; |
There was a problem hiding this comment.
use an import rather than referring to duke.TaskType
src/main/java/Task.java
Outdated
| public class Task { | ||
| duke.TaskType taskType; | ||
| boolean isDone; | ||
| String string; |
There was a problem hiding this comment.
is there a better name than string? this isn't very informative
src/main/java/Storage.java
Outdated
|
|
||
| public ArrayList<Task> load() throws DukeException { | ||
| File file = new File(filePath); | ||
| ArrayList<Task> list = new ArrayList<>(); |
There was a problem hiding this comment.
i think my tutor mentioned that 'list' as a collection name isnt very intuitive and informative.
There was a problem hiding this comment.
Very clean and readable code! goodjob! However, i noticed that you use 8 spaces for indentation while the convention is to use 4. also, remember to add javadocs for public methods!
other than that, code is clear and concise!
// edit: sorry my internet lagged and i submitted two reviews oops
Package classes into sub directories and imported relevant classes. Duke: Remove unused code (from previous iterations of GUI) from duke class. Remove Constructor with single param filename. Update main to use no param constructor. Made changes to the JavaDocs. Storage: Add switch statement for Storage class Task: used unicode number instead of the cross and tick characters.
Branch a code quality
# Conflicts: # src/main/java/Duke.java # src/main/java/Parser.java # src/main/java/storage/Storage.java # src/main/java/task/Task.java
# Conflicts: # src/main/java/Duke.java # src/main/java/Parser.java # src/main/java/storage/Storage.java # src/main/java/task/Task.java
No description provided.