-
Notifications
You must be signed in to change notification settings - Fork 437
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
[Pang Wai Kye] iP #24
base: master
Are you sure you want to change the base?
Conversation
…and FileHandler.java
Add save file capabilities and minor improvements
Merge which adds Regex handling, modifying functions and classes to accomodate LocalDate class
…andler -> Storage, DukeList -> TaskList. Improve regex expression for Storage. Abstract contents of Duke main to run() function and various classes.
…ask, under one main package duke.
…ges. Improve regex expression in Parser.
…on the words in the description.
# Conflicts: # src/main/java/duke/command/Parser.java # src/main/java/duke/storage/Storage.java # src/main/java/duke/task/Task.java # src/main/java/duke/task/Todo.java
# Conflicts: # src/main/java/duke/task/TaskList.java
…nt after merge of the 3 branches.
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 in general. There are some small problems that need to be addressed.
|
||
/** | ||
* Checks if the command to exit has been issued | ||
* @return True if 'bye' command has been issued, False otherwise. |
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.
Will it be better if there can be a new line between the description of the method and the description of the parameter and the description of the result?
/** | ||
* The function that processes the incoming command and executes different | ||
* functions depending on the command. | ||
* @param command String command provided by the user. |
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.
Will it be better if there can be a new line between the description of the method and the description of the parameter and the description of the result? I have noticed similar problems in several other places.
case ("todo"): | ||
case ("deadline"): | ||
case ("event"): | ||
return this.processTask(com, task, date); |
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.
Will it be better to include a default section here?
* @return String result of the provided command. | ||
* @throws DukeException A custom Exception that carries a message for the user if thrown. | ||
*/ | ||
public String processCommand(String command) throws DukeException { |
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.
Will it be better to give a more descriptive name for the method, such as identifying the command elements and evaluating the command?
if (command.equals("list")) { | ||
return this.list.toString(); | ||
} | ||
if (command.equals("hello")) { |
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.
Will it be better to include the keyword else here?
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.
As mentioned by HCY123902, could this have been a series of if-else statements instead?
* Checks if the /data directory exists. | ||
* @return True if it exists, False otherwise. | ||
*/ | ||
private boolean dirExists() { |
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.
Will it be better to give a more descriptive name to the method, such as the name is directory present?
* Checks if the /data/list.txt file exists. | ||
* @return True if it exists, False otherwise. | ||
*/ | ||
private boolean fileExists() { |
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.
Will it be better to give a more descriptive name to the method, such as the name is file present?
* Gets the TaskList that has been saved in list.txt File. | ||
* @return TaskList. | ||
*/ | ||
public TaskList getList() { |
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.
Will it be better to give a more descriptive name to the method and include the sources of the task list in the name?
src/main/java/duke/task/Event.java
Outdated
*/ | ||
public class Event extends Task { | ||
|
||
LocalDate date; |
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.
Will it be better to give an accessor to the variable here?
"delete <index> - deletes the specified task from the list"; | ||
} | ||
if (matcher.find()) { | ||
String com = matcher.group(1); |
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.
Will it be better to give a more descriptive name and include the information on which part of the command it represents?
…of Duke into the GUI.
…rove GUI. In UI class, showWelcome() now returns a String.
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.
Largely good style!
if (command.equals("list")) { | ||
return this.list.toString(); | ||
} | ||
if (command.equals("hello")) { |
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.
As mentioned by HCY123902, could this have been a series of if-else statements instead?
|
||
import java.time.LocalDate; | ||
import java.time.format.DateTimeFormatter; | ||
import java.time.format.DateTimeParseException; |
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.
Should there be a line break between the import statements here?
return "Hi! I'm Duke! Pleasure to meet you :)"; | ||
} | ||
if (command.equals("bye")) { | ||
this.isExit = true; |
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.
is isExit, rather than this.isExit, sufficient?
* @return String result that describes the item added | ||
* if successful. | ||
*/ | ||
public String addItem(Task item) { |
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.
Should the method be renamed as addTask instead to be consistent?
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.
Yes, I totally agree with kerkpy's input
* @return String result that describes the deleted Task | ||
* if successful. | ||
*/ | ||
public String deleteItem(int 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.
As per addItem, could this be deleteTask instead of deleteItem
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.
It would be better if you could do more abstraction per se, but all things are good. Just that there are some naming that could cause confussion.
* @return String result of the function executed. | ||
* @throws DukeException A custom Exception that carries a message for the user if thrown. | ||
*/ | ||
private String processTask(String com, String task, String date) throws DukeException { |
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.
Hi, I think the naming of your paramaters could be made more clearer, such that com --> command. Sometimes it is just confusing to read a contracted or shortened word.
} | ||
|
||
/** | ||
* Private method that gets the list.txt file if present, else a new |
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 comments seems good and it would be better if it could start with verbs instead following the Coding Standard
while (scanner.hasNextLine()) { | ||
Matcher matcher = pattern.matcher(scanner.nextLine()); | ||
if (matcher.find()) { | ||
boolean done = matcher.group(2).equals("1"); |
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 would be better to set as isDone instead of done, as it is a boolean data type. It is just so that the variable is more descriptive
* @param task The description of the task. | ||
* @param date The LocalDate object that specifies the due date. | ||
*/ | ||
public Deadline(String task, LocalDate date) { |
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 for task description would be better to be named taskDescription rather than just task.
*/ | ||
public class TaskList { | ||
|
||
private List<Task> list; |
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 would be better set as lists rather than list. It is just so that it is more descriptive
* @return String result that describes the item added | ||
* if successful. | ||
*/ | ||
public String addItem(Task item) { |
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.
Yes, I totally agree with kerkpy's input
…d in error handling.
…s DukeException upon error in creation of list.txt save file.
A - Assertions
A - CodeQuality
…ble to add notes yet.
…e and "note" functionality which allows users to write the note. Minor bug fixes in Parser to prevent non-numbered inputs for index related commands.
…xist in the list results in program failure.
No description provided.