-
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
[Kang Su Min] iP #329
base: master
Are you sure you want to change the base?
[Kang Su Min] iP #329
Conversation
There is a basic greeting feature but no echo and exit features. A more personlized chat bot with a unique logo and tone will be more fun. Let's * add echo and exit features * change the logo and tone of the chat bot
The chat bot can only echo commands, is unable to store user input. Adding add and list feature would allow chat bot to store user input and display past input of user on demand, engaging user. Let's * implement add and list feature
User can only add tasks but cannot mark a task as done. Adding this future allows users to give 'done' command which makes chat bot more useful to users when keeping track of their tasks. Let's * add 'mark as done' feature * add error catching methods to show error message when done command is executed with out of bounds index or a non-integer
User can only add one type of task, and cannot differentiate between different types of tasks in their list. Adding child classes to Task - Todo, Event, Deadline would help user add different types of tasks to their list. Printing the number of tasks in the user's list after addition of every task will also improve user experience. Let's * implement child classes of Task - Todo, Event, Deadline * add a method to print the number of tasks in the message after adding every task to the list
Chat bot has no automated testing function and requires testing through individual commands. Adding automated testing function would allow faster and more efficient testing of the chat bot. Let's * add automated text UI testing to the program using I/O redirection technique
Chat bot is unable to handle invalid user input and throws exception, which affects user experience. Implementing a DukeException to handle invalid input would improve user experience as customisation of error messages is possible. Let's * create a DukeException class to deal with invalid input * handle invalid done command with out of bounds index * handle invalid task command * handle empty todo command
Chat bot only allows for adding tasks but does not support deletion of tasks. Adding a delete feature to the chat bot would make it more useful to users. Let's * add a new delete method to handle delete command * add a new delete message to print to user when a task is deleted
Code in main Duke class is long because of multiple boolean methods to check type of command. Using enums will shorten code and make it more readable. Let's * implement InputCommands enum to represent command type
Any changes made to the list of tasks does not get saved, and every run of the program would give an empty list at the start, which is not useful to the user. Adding save feature would allow the program to save the changes to the list of tasks from the previous run of the program. Let's * save the list of tasks in a duke.txt file under data folder every time there are changes to the list of tasks * read data from the duke.txt file at the start of the program run so that the data from previous program can be called and used in the program
The chat bot accepts the date/time as a string input from user, which means meaningful storage of dates is not possible as there is no conversion of the string input to date object. Importing LocalDate class and storing string input by users as a Date object will help chat bot understand date better and do more meaningful operations with date. Let's * import LocalDate class and store deadline/event dates as a LocalDate object in task objects * implement new command that prints deadlines/events occurring on the date given as string input by user
The changes to the code are in branch-Level-7 Merging branch-Level-7 to master branch will update the code in master branch to the one in branch-Level-7 and join them back into one master branch. Let's * merge branch-Level-7 to master branch
There are merge conflicts between branch-Level-8 and master branch. Resolving conflicts and merging the two branches would update my code in master branch. Let's * resolve conflicts and merge branch-Level-8 to master branch
The methods are not abstracted enough. Adding more OOP concepts into my code by abstracting classes and methods will improve readability. Let's * create Parser, Storage, TaskList, Ui classes to abstract code * create *Command classes that inherit from abstract Command class
Projet code is unorganised because it is not packaged properly. Packaging code into different packages will make the code more organised. Let's * create package duke * create sub-packages task and command to group different files together
Code does not have any JUnit tests. Adding JUnit tests would improve efficiency when finding errors in code. Let's * add some JUnit tests to test creation of file strings for Todo, Event and Deadline tasks. * add JUnit tests to test whether DukeException is thrown for invalid add commands
Code now has no description to explain what the methods and classes do. Adding JavaDocs would help readers understand code better. Let's * add JavaDocs to public classes and methods
Parts of code do not follow coding standards. Adding coding standards would increase readability of code. Let’s * add coding standards to code
Code does not contain class and methods for keyword search function. Adding find feature would allow users to find tasks more efficiently. Let's * add create FindCommand class to represent find command and add methods accordingly.
Branch for JavaDoc is not yet merged into master branch. Let's * merge branch-A-JavaDoc into master branch to include JavaDoc in master branch
Branch for coding standard not yet merged into master branch. Let's * merge branch-A-CodingStandard into master branch
Branch-Level-9 find feature not yet merged into master branch. Let's * merge branch-Level-9 into master branch
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, your code is neat and readable. There isn't much problem with the OOP principles either. However, you might want to avoid magic numbers and use more intuitive name. Furthermore, don't forget to write access modifiers when declaring your attributes! Good job on your iP!
P. S. I like your bot's personality (reminds me of the Minions!😆) Looking forward to the final product.
src/main/java/duke/Storage.java
Outdated
List<Task> tasks = new ArrayList<>(); | ||
String filePath; | ||
File dataFile; |
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 it is better to add access modifier to your variables (private, preferably) to comply with coding standard (and encapsulation) 😄
src/main/java/duke/Duke.java
Outdated
public static void main(String[] args) { | ||
new Duke("data/duke.txt").run(); | ||
} | ||
} |
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's missing the newline at the end of the file. You might want to consider doing checkstyle in IntelliJ as it highlights all coding style violation! (makes our life easier) 😄
*/ | ||
public class DukeException extends Exception { | ||
|
||
String message; |
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.
In my opinion, you don't need the message attribute since Exception has it.
@Override | ||
public String toString() { | ||
return "Apple Pineapple!! " + message; | ||
} |
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.
Following the previous comment, there is no need to write toString() method since Exception class already has a built-in function getMessage() to fetch the error message.
src/main/java/duke/Parser.java
Outdated
import duke.command.Command; | ||
import duke.command.ExitCommand; | ||
import duke.command.AddCommand; | ||
import duke.command.DoneCommand; | ||
import duke.command.DeleteCommand; | ||
import duke.command.InputCommand; | ||
import duke.command.GetCommand; | ||
import duke.command.ListCommand; | ||
import duke.command.FindCommand; |
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 good that you group the import statements consistently. However (this is just nitpicking, not compulsory), it looks neater if you write them in lexicographical order.
/** | ||
* Represents command that is specific to the get command. | ||
*/ | ||
public class GetCommand extends Command { |
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.
May I ask what is GetCommand for? Correct me if I'm wrong but assuming this is for finding task, perhaps you might want to consider renaming this class to FindCommand to avoid confusion. Since get is a bit ambiguous (can also be interpreted as a verb).
GET_TASK, | ||
FIND, |
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.
May I ask what is the difference between both of these commands? Maybe you want to rename it something more intuitive (SHOW, FIND_KEYWORD, FIND_DATE, etc) ?
/** | ||
* Represents task of user | ||
*/ | ||
public class Task { |
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 might want to change this class to an abstract class because it doesn't make sense to create a new a Task. It is algo a good practice in OOP because it reduces code repetition.
src/main/java/duke/task/Task.java
Outdated
String dateString; | ||
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.
Don't forget the access modifier! 😄 It is important as attributes should be declared private when possible and neglecting access modifier makes the attributes implicitly public.
src/main/java/duke/task/Task.java
Outdated
* Returns string representation of task object to be stored in file | ||
* @return string representation of task object for file storage | ||
*/ | ||
public String toFileString() { |
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 might want to consider renaming this to comply with the coding standard. Method name should be a verb and intuitive.
# Conflicts: # src/main/java/duke/Parser.java # src/main/java/duke/Storage.java # src/main/java/duke/Ui.java # src/main/java/duke/command/Command.java # src/main/java/duke/task/TaskList.java
Runtest.sh actual and expected output differs. Updating expected output file will fix the difference in output. Let's * Update EXPECTED.TXT
Project now does not have checkstyle configurations. Adding checkstyle configurations will allow users to run checkstyle locally. Let's * add checkstyle configurations
Code has a few coding style violations. Updating code to follow checkstyle would make the code follow the conventional coding style and would make code more readable. Let's * use checkstyle on all files to find checkstyle errors * update code accordingly to follow checkstyle
branch-A-Checkstyle is not yet merged to the master branch. Merging would update master branch to follow checkstyle as well. Let's merge branch 'branch-A-Checkstyle' to master branch.
The Chat bot now is a command line app. Adding GUI would make it more visually appealing and interactive. Let's * add GUI features using javafx
branch-Level-10 not yet merged into master branch. Merging branch would incorporate GUI features into master branch. Let's merge branch-Level-10 into master branch.
Program does not have assertions to verify the truth of certain assumptions. Adding assertions would help document important assumptions that should hold at certain points in code. Let's add assertions to program.
Add assertions to program
Code does not comply with some coding standards. Refactoring code to comply with coding standards would improve quality of code. Let's * check code quality using Java coding conventions * refactor code where necessary
Improve code quality
Program unable to detect duplicates. Let's add methods to detect duplicates.
Program does not use lambdas and streams. Using lambdas and streams will shorten code. Let's add lambdas and streams to program where applicable.
The GUI of the chat bot can be improved. Adding design that is in line with the personality of the chat bot would make user experience more enjoyable. Let's * improve on the GUI of the chat bot by adding background, color, changing font family and size
Adding User Guide helps users Let's add User Guide
User Guide has no images. Adding a screenshot of the chat bot will make User Guide more readable and enjoyable. Let's * add screenshot of chat bot * add credits to user guide
Features header not showing. Let's try to remove the space to fix the problem.
There are several mistakes in the User Guide. Ui.png also has lines at the side. Updating user guide and replacing image would improve user guide. Let's * update user guide * update Ui.png
Commands unable to run properly when app is opened for the first time. Let's fix null pointer error in the Storage class.
No description provided.