-
Notifications
You must be signed in to change notification settings - Fork 116
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
[Nigel Yeo] iP #18
base: master
Are you sure you want to change the base?
[Nigel Yeo] iP #18
Conversation
echos commands entered by the user, and exits when the user types the command bye
Stores the tasks that user inputs and outputs the list when user enter command "list"
Currently, the bot is only able to store the list of task being done but there is no way of allowing users to track if task is done Added marking symbols with "X" between "[ ]" braces after the indexing of task to allow user to track if task is done. User is able to ivoke markTask() and unmarkTask() functions through entering keywords such as "mark" and "unmark" respectively followed by the index of the task in list() function to mark and unmark the tasks.
The code is only able to mark and unmark indexes up to 9 due to the way the function is implemented. The number of task that the bot is implemented for is up to 100 meaning the indexes goes up to 99. Thus it needs to be changed to accomodate this. The code is rectified and fixed to fit the constrain.
Currently, we are only able to add tasks through the bot to the list and this is not very effective as there may be differing types of tasks that require more specific details such as due dates or event timings The bot now is able to have 3 types of Task catergorisation namely, Todo, Deadline Task and Event. Users need to input in "todo", "deadline" and "event" keywords at the start of the input before typing the task to categorise them. Deadline tasks are able to set a deadline for the task by adding a deadline after the "/by" keyword in the input after the task and for events it is able to give a range of time with the "/from" and "/to" keywords after the input of the task.
Test files are correct as of 1 September 2024 after level-4 increment done
This reverts commit e5bb5b4.
Test files are correct as of 1 September 2024 after level-4 increment done
Abstracted methods and conform to naming conventions to improve code readability
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 easy to read! There are two changes to take note of, one for line wrapping and another for the default case in the switch statement 👍
src/main/java/Event.java
Outdated
int startingIndexOfEventStart = input.indexOf("/from") + 6; | ||
int endingIndexOfEventStart = input.indexOf("/to") - 1; | ||
int startingIndexOfEventEnd = endingIndexOfEventStart + 5; |
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 named these variables, it helps me understand how this part of the code works :)
src/main/java/Legin.java
Outdated
default: | ||
echo(input); |
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 break; after echo(input)?
src/main/java/Legin.java
Outdated
switch (command) { | ||
case "bye": | ||
saidBye = true; | ||
break; | ||
case "list": | ||
list(); | ||
break; | ||
case "mark": | ||
indexOfTaskToMark = Integer.parseInt(words[1]); | ||
markTask(indexOfTaskToMark); | ||
break; | ||
case "unmark": | ||
indexOfTaskToMark = Integer.parseInt(words[1]); | ||
unmarkTask(indexOfTaskToMark); | ||
break; | ||
case "todo": | ||
addTodo(input); | ||
break; | ||
case "deadline": | ||
addDeadline(input); | ||
break; | ||
case "event": | ||
addEvent(input); | ||
break; |
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.
This part of the code is really clean, with good indentations! :D
src/main/java/Legin.java
Outdated
System.out.println("Bye " + | ||
Character.toString(0x1F44B) + | ||
". Hope to see you again really soon! " + | ||
Character.toString(0x1F608)); |
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 this indentation for wrapped lines be 8 spaces instead of 4 spaces?
System.out.println("Bye " + | |
Character.toString(0x1F44B) + | |
". Hope to see you again really soon! " + | |
Character.toString(0x1F608)); | |
System.out.println("Bye " + | |
Character.toString(0x1F44B) + | |
". Hope to see you again really soon! " + | |
Character.toString(0x1F608)); |
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 the code is kept simple and neat down the code, and I am inspired to do the same for mine. Great job!
src/main/java/Legin.java
Outdated
public static void horizontalLine() { | ||
System.out.println("______________________" + | ||
"______________________________________"); | ||
} |
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 to do this! as one might want to print lines for every output, doing this is logical so you can call it when needed and keep things neat.
src/main/java/Legin.java
Outdated
private static void printAddedTaskMessage() { | ||
horizontalLine(); | ||
System.out.println("Got it. I've added this task:"); | ||
System.out.println(tasks[currentTaskCount]); | ||
currentTaskCount++; | ||
System.out.println("Now you have " + currentTaskCount + " tasks in the list."); | ||
horizontalLine(); | ||
} |
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 job doing this as it prevents repeated lines of code! I see that you call this quite often below :)
src/main/java/Legin.java
Outdated
public static void main(String[] args) { | ||
greet(); | ||
runBot(); | ||
bye(); | ||
} |
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 was surprised when I see this, good to see that all you managed to keep things simple here
src/main/java/Legin.java
Outdated
default: | ||
echo(input); |
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 didn't think of doing a default case. good job!
Currently, the bot is unable to verify or validate the input of the users which may lead to various unexpected outcomes and behaviours that is not intended. The bot thus added try catch statements for customised exceptions for the bot. The exceptions are as follows: 1. When user inputs no task for his/her todos, events, deadlines 2. When user inputs an invalid command 3. When user missed out parameters for a given event or deadline The bot will catch these human errors and output them to the user so he/her can edit the input and give the bot the expected one.
Currently, the bot is unable to delete the task which is is required by the user when he/she wants to remove a task that is no longer relevant. The current implementation is of a fixed size array of 100 which makes it inconvininet to delete task efficiently. The bot now runs with an ArrayList instead of fixed length array to leverage on the remove function of the collection. This allows easy deletion of tasks. The user can use the delete function with the following input 'delete [task number]'.
Currently, users cannot save their task data on the bot and will restart to an empty task list everytime they run the bot, this defeats the purpose of having a bot that tracks your daily and future tasks. Filewriting to a local file is implemented such that the file loads the user's data from the file into the bot everytime it is ran and updates the file each time the bot shuts down using the bye funciton.
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 is a very good attempt to follow all the guidelines.
Do make sure you get rid of magic strings / numbers and change some of the naming for methods and boolean variables.
src/main/java/Legin.java
Outdated
import task.Task; | ||
import task.Todo; | ||
import java.io.IOException; | ||
import java.util.ArrayList; |
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 job on providing explicit imports. Perhaps you would want to order them in a collated order e.g. java.util.ArrayList is together with java.util.Scanner, since both comes from java.util package.
Note that this is part of Java coding standard - Intermediate.
src/main/java/Legin.java
Outdated
private static int currentTaskCount = 0; | ||
private static final String FILE_PATH = "./tasklist.txt"; | ||
|
||
public static void horizontalLine() { |
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.
You would want to use verbs to name a method instead of nouns.
public static void horizontalLine() { | |
public static void printHorizontalLine() { |
src/main/java/Legin.java
Outdated
public static void addTextDataToArray(String[] words) throws LeginEmptyTaskException { | ||
String taskType = words[0]; | ||
switch(taskType) { | ||
case "T": |
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 you prevent the usage of magic numbers and characters?
src/main/java/Legin.java
Outdated
while (s.hasNext()) { | ||
String[] words = s.nextLine().split("\\|"); | ||
addTextDataToArray(words); | ||
} |
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.
Please look out for indentations.
src/main/java/Legin.java
Outdated
} | ||
} | ||
|
||
public static void 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.
Perhaps be more specific on the naming of this method.
src/main/java/Legin.java
Outdated
} | ||
|
||
public static void runBot() { | ||
boolean saidBye = false; |
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 name the boolean with a prefix "has"?
src/main/java/Legin.java
Outdated
inputTextFileData(); | ||
horizontalLine(); | ||
} | ||
public static void bye() throws IOException { |
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.
Similar to a previous issue, would you like to use verbs for this method instead?
src/main/java/task/Deadline.java
Outdated
return input.substring(input.indexOf(" ") + 1, input.indexOf("/by") - 1); | ||
} | ||
|
||
private static void validityCheck(String input) throws LeginMissingParamsException, |
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.
You would want to use a verb here instead.
private static void validityCheck(String input) throws LeginMissingParamsException, | |
private static void checkValidity(String input) throws LeginMissingParamsException, |
src/main/java/task/Deadline.java
Outdated
private static void validityCheck(String input) throws LeginMissingParamsException, | ||
LeginEmptyTaskException { | ||
int indexOfBy = input.indexOf("/by"); | ||
if (indexOfBy == 9) { |
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.
Would you be able to remove the magic numbers in this file (as well as other files)?
src/main/java/task/Deadline.java
Outdated
this.by = input.substring(startingIndexOfDueDate); | ||
} | ||
|
||
public Deadline(String description, String by) throws LeginEmptyTaskException { |
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.
What does "by" refers to? It would be good if this is named more appropriately.
src/main/java/Legin.java
Outdated
break; | ||
case "E": | ||
tasks.add(new Event(words[2], words[3], words[4])); | ||
break; |
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.
You may want to prevent error-prone code by adding default branch.
Currently, the coding structure lacks seperate containers to group the type of functions in the Main class. This makes the code untidy and unorganised. Additional classes Ui, Parser, TaskList and Storage are created. Ui class contains of all the required print statements for the entire program and all classes use Ui methods to print or take in input. Parser class deals with user input and parses it into command, the actual task, date and more. Parser mainly calls TaskList methods accordingly after retrieving the relevant details from the input. TaskList class stores all the Task for the session and its methods change the current tasklist such as adding task, deleting task, marking them and more. Storage class is used in the initial start up of the programme as well as before the program closes, it loads the text file information into the program as well as save its state before the user exits.
Currently, the scope of usage remains to smaller number of task but as we expand the usage to accomodate higher number of tasks, the need for a find function becomes more apparent to ease the usage of finding tasks when the list gets long. Find function is implemented and ivoked with the command 'find' followed by the words that the user wishes to match. The output list all matching tasks with those keywords.
Add find function to bot
Add JavaDoc comments for the entire code base
Repackage files and clean up code
No description provided.