-
Notifications
You must be signed in to change notification settings - Fork 482
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
[owenyeo] IP #522
base: master
Are you sure you want to change the base?
[owenyeo] IP #522
Conversation
…bsite. Added functionality to rename chatbot.
Added functionality to read User input Added ability to exit chat when User Input is "bye" Provided documentation. Removed Message subclass due to redundancy.
Added functionality to add items into list
Added a Command Enum for cleaner code Added new classes Task, Event, Todo, Deadline
…nException. Added error handling in ChatBot.
Added InvalidIndexException for number errors in mark, unmark, and delete.
Added saveTasks to save data on a text file. Added loadTasks for potential future use.
Improved formatting of toSaveString in ChatBot.java Fixed bug where type of task is not correctly shown. Removed loadFile due to redundancy.
Added functionality to reformat DateTime when printing.
Added functionality for Storage to load previous lists. Added SaveFileNotFound exception
Added documentation for all new classes. Fixed bug when marking tasks as done and reloading it in the next session.
Previous version of ChatBot did not have support for Gradle. Support for gradle is required as we ramp up automated unit testing. Gradle support is therefore added, with JUnit tests coming in the next commit. Gradle is used for its ease of use and compatibility with JUnit. Fixed bugs regarding error messages not printing out.
No automated unit testing was implemented in this project. This led to time inefficiency as I had to manually test each component one by one. Therefore, JUnit tests are being implemented to test the various components in this project. Currently, I am implementing only tests for TaskList and Parser. More will be implemented as required. Fixed a bug regarding InvalidDescriptionException not being thrown when an empty description is passed through.
No coding convention was adhered to. This causes code to be less readable by others. Thus, from now on the project will adhere to Java Coding Convention. Checkstyle will be implemented and consistent checks from developer side will continue to ensure this.
There is no find command currently in the chatbot. This makes it hard to look for tasks that the user may specifically is looking for. A find command is thus added and is currently working as per developer's testing.
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.
LGTM, just some nits on naming etc
src/main/java/ChatBot.java
Outdated
|
||
//Enum Command to make code cleaner and allow for the use of | ||
//switch case statements. | ||
private static enum 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.
Maybe consider using Commands instead of Command?
src/main/java/ChatBot.java
Outdated
public void addToList(String taskString, Command command) | ||
throws InvalidDescriptionException { | ||
switch(command) { | ||
case ADD_TODO: |
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.
indentation of case should be same as switch i think?
src/main/java/ChatBot.java
Outdated
list.add(new Event(eventLabel.trim(), eventParts2[0].trim(), eventParts2[1].trim())); | ||
saveTasks(); | ||
} catch (IndexOutOfBoundsException e) { | ||
throw new InvalidDescriptionException("Are you stupid? Can you follow instructions?"); |
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.
haha this actually made me laugh. q funny
src/main/java/ChatBot.java
Outdated
* @param listNum Index of the item of the list to delete. | ||
*/ | ||
public void delete(int listNum) { | ||
int index = listNum - 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.
Perhaps be more descriptive? index of (indexDone)?
src/main/java/Event.java
Outdated
* @author Owen Yeo | ||
*/ | ||
public class Event extends Task{ | ||
private LocalDateTime from; |
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 use a more descriptive name? eg fromTime? from feels abit too general it could be from (location) instead of from (time)?
Project has no checkstyle support, which makes it difficult to check for errors pertaining to coding convetion. Thus, checkstyle support has been added to automate checking and ensure better code quality.
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, some minor changes with naming and comments. LGTM!
src/main/java/chatbot/ChatBot.java
Outdated
} | ||
} | ||
|
||
public void 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.
Perhaps a JavaDoc comment for this method?
* | ||
* @return false | ||
*/ | ||
public boolean isExit() {return 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.
Should this method follow Egyptian style braces instead?
*/ | ||
@Override | ||
public void execute(TaskList tasks, Storage storage, Ui ui) | ||
throws InvalidIndexException { |
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 wrapped line be indented by 8 spaces instead?
@@ -0,0 +1,9 @@ | |||
package chatbot.exceptions; | |||
|
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 a JavaDoc comment here to describe the class.
switch(command) { | ||
case BYE: | ||
return new Bye("", CommandType.BYE); | ||
|
||
case DISPLAY_LIST: | ||
return new DisplayList("", CommandType.DISPLAY_LIST); | ||
|
||
case MARK: | ||
return new MarkItem(inputStrings[1], CommandType.MARK); | ||
|
||
case UNMARK: | ||
return new UnmarkItem(inputStrings[1], CommandType.UNMARK); | ||
|
||
case ADD_TODO: | ||
return new AddToDo(inputStrings[1], CommandType.ADD_TODO); | ||
|
||
case ADD_DEADLINE: | ||
return new AddDeadline(inputStrings[1], CommandType.ADD_DEADLINE); | ||
|
||
case ADD_EVENT: | ||
return new AddEvent(inputStrings[1], CommandType.ADD_EVENT); | ||
|
||
case DELETE: | ||
return new DeleteItem(inputStrings[1], CommandType.DELETE); | ||
|
||
case FIND: | ||
return new FindTask(inputStrings[1], CommandType.FIND); | ||
|
||
default: | ||
throw new InvalidCommandException("Don't be stupid, speak english."); |
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 clean this looks. Keep it up!
/** | ||
* | ||
*/ | ||
@Override | ||
public String toSaveString() { | ||
return "D " + super.toSaveString() + " | " + originalString; | ||
} | ||
|
||
/** | ||
* | ||
*/ |
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 a JavaDoc comment here? Since the methods are overriden, I think removing the comment is fine too!
src/main/java/chatbot/ui/Ui.java
Outdated
public void bye() { | ||
print(new String[] {"Bye. Have a bad day you doofus."}); | ||
} |
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 a method name that starts with a verb, for example printGoodbye
would be more descriptive?
For your reference:
Names representing methods must be verbs and written in camelCase.
There is no GUI for ChatBot, which is not ideal for User Experience. A UI was therefore added using JavaFX, and is currently working well as per developer testing and JUnit tests.
ChatBot has no support for FXML, which makes GUI design clunkier and more tedious. FXML Support thus has been added to facilitate GUI design. Original functions seem to be working well as per unit testing and developer tests.
Project does not have any assert statements, meaning that potential issues with logic may go unnoticed. Adding assertion statements thus will allow for better developer testing and ensure code works as intended. As a step towards bettering code quality, assertion statements have been added to some classes where errors tend to happen, e.g. tasklist and tasks with timings.
Code has some minor violations in coding standards throughtout the project. This leads to lower readability of code and thus makes it hard to debug. Checkstyle is thus used to detect such errors, and code is screened to correct any violation detected.
Chatbot did not recognise and handle duplicate entries of the same task. This led to clunky UX as users added multiple entries of the same task. Added functionality to inform users of duplicate entries when adding new tasks, to list duplicates, and to delete all duplicates if any. Fixed a bug regarding delete where the wrong number of remaining tasks left is shown.
README formatting was wrong and looks clunky. Fixed remaining issues.
File path is hardcoded into chatBot, causing .jar files to not save tasks properly. Bug has been fixed.
App could not run on windows systems due to FXML compatibalitity issues. Imported packages for Windows and Linux to ensure cross-platform compatibality Brushed up on some header commands.
README is outside of docs folder, making it messy. Moved README to docs folder.
ChatBot
ChatBot frees your mind of having to remember things you need to do. It's,
All you need to do is,
And it is FREE!
Features:
Look at the charismatic Bot's introduction!