-
Notifications
You must be signed in to change notification settings - Fork 462
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
[Johannes] iP #498
base: master
Are you sure you want to change the base?
[Johannes] iP #498
Conversation
…in a new prompt feedback
…tion of chatbot to minimise code repetition.
…em as a list when the string list is passed as an input
Added checklist functionality and modified storeman method to accomodate new function. Added listOut method to generalise listing out of tasks, to minimise code repetition.
…implemented within storeman method). replaced contains with startsWith where appropariate to ensure correctness of program when running.
…unknown or in the wrong format, to be caught by main. Main will restart duke upon catching the exception and print the message of the error to console right before.
… the process of testing
Merge pull request 1 from temp branch to master branch
…epresentation of Task and Deadline
storage.txt is a cached file by Duke to save the list of Tasks by user in previous run. storage.txt changed every run and there's no need to track the file in repo. Hence, should be include in .gitignore as a file to be ignored by git tracking
The classes are not packaged and organised. We need to organise our files better, into duke package, utility package and dukeexception package. Hence we add package statements for each file according to where they are organised and import package statements accordingly.
No JUnit test to test correctness of classes. We want to test the toString methods of Event and Todo classes to ensure correctness. Added JUnit tests for the toString methods of Event and Todo classes using JUnit.
Currently, Duke does not have functionality to find matching tasks in the TaskList when requested by client We want to allow client to find their matching tasks when requested. Added conditionals in Parser and methods in TaskList and Ui classes to support this functionality.
…ove method not used from Ui class. askForPath is a method from the Ui class which is not used. There is no task to create a fat jar via build.gradle. askForPath affects readability and adds extra lines of unnecessary codes. We also need a task to create a fat jar via build.gradle. We should remove askForPath method sicne it is nto used and add task to build.gradle in order to automate the creation of fat jar file.
…t instead. Added Java documentation. add method is in Storage class but it deals with adding tasks into the task list. There is no Java documentation. TaskList class should be handling this operaton. Java documentations should also be added for the classes. Transfer the add method to the TaskList class from the Storage class. Added Java documentation for the classes of the program.
Previously, Checkstyle was not implemented to check for coding standard violations. There are many Checkstyle coding violations upon implementing the Checkstyle Feature via Gradle. Fixed many of such violations to meet the Checkstyle coding standards.
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 general, I think your code looks good already. There's some coding standard violations listed below, but other than that, LGTM!
private TaskList tasks; | ||
private Ui ui; | ||
private ArrayList<Task> al; | ||
private Parser parse; |
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 the variable al and parse can be renamed to a more descriptive object name?
/** | ||
* Defines the Date time format to read in the commands passed by client for processing. | ||
*/ | ||
protected static final DateTimeFormatter DTF = DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm"); |
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.
Nice abstraction!
try { | ||
new Duke("storage.txt").run(); | ||
} catch (DukeException de) { | ||
System.out.println("--------------------------------------\n"); |
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 dashed line is used multiple times. Maybe it can be defined as a constant?
@@ -0,0 +1,83 @@ | |||
package dukechatbot.utility; | |||
import java.io.*; |
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 you can list all the imported classes explicitly?
* Encapsulates the array list associated with the instance of | ||
* TaskList | ||
*/ | ||
private ArrayList<Task> tl; |
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 can rename this variable to make it more descriptive?
if (uncap.startsWith("deadline")) { | ||
int idOfSlash = str.indexOf('/'); | ||
if (idOfSlash - 9 == 0) { | ||
this.ui.showDescEmptyError("deadline"); |
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 this deeply nested if-else statement make the code quiet hard to read
/** | ||
* Defines the array list that is associated with the program run. | ||
*/ | ||
private ArrayList<Task> tl; |
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 this variable can also be renamed to something more descriptive.
Duke was a CLI application. To make it more appealing, we should implement a basic GUI. Added basic GUI functionality using JavaFx.
There was no documentation for the new classes previously. We need documentation to explain the new classes to fellow programmers. Documented the different classes implementing the GUI.
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 good but some minor coding standard violations.
private Storage storage; | ||
private TaskList tasks; | ||
private Ui ui; | ||
private ArrayList<Task> al; |
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 there's no need for another ArrayList when theres already a TaskList
if (uncap.startsWith("unmark")) { | ||
int i = Integer.parseInt(String.valueOf(uncap.charAt(7))); | ||
response = tasks.unmark(i, ui); | ||
} else if (uncap.startsWith("mark")) { |
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.
Deeply nested if else loops are quite confusing, perhaps could make use of switch statements instead?
@@ -0,0 +1,83 @@ | |||
package dukechatbot.utility; | |||
import java.io.*; |
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 import classes explicitly
if (String.valueOf(ln.charAt(4)).equals("X")) { | ||
done = true; | ||
} | ||
if (tag.equals("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.
Perhaps this parsing of the command to determine how to create the Todo should be done in the Parser class?
String tag = String.valueOf(ln.charAt(1)); | ||
String desc = null; | ||
int id = -1; | ||
boolean done = 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.
Could use isDone as variable name instead
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 think your code is clean and easy to understand!
System.out.println("--------------------------------------\n"); | ||
System.out.println(io.getMessage()); | ||
System.out.println("--------------------------------------\n"); |
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 this can be abstracted into a utility method, for example formatErrorMessage
?
} | ||
|
||
/** | ||
* lists out the tasks in the task list to 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.
This is not stated explicitly in the coding standard docs but I think the first letter should be capitalised?
/** | ||
* lists out the matching tasks in the task list that meets | ||
* the user's find query. | ||
* @param tl containing the tasks store in the 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.
Could add an empty line between description and parameter section.
if (file.exists()) { | ||
this.load(); | ||
} else { | ||
if (file.createNewFile()) { | ||
this.load(); | ||
} else { | ||
throw new IOException("File failed creation!"); | ||
} | ||
} |
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.
Since if the named file already exits createNewFile()
does nothing and return false, maybe this part can be further simplified?
Besides, the IOException
will be thrown if the file already exits, if that's the intended behaviour maybe it would be clearer if the error message indicate that file already exits?
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.
Very good
save was a non-static method of the Storage class. The save() should be static method since it's associated to Storage class not the instance. Changed save to be static and modified code to work with this change in implementation.
Build.gradle does not point to Launcher class which meant the jar file does not use javaFX. To create a Jar file using JavaFX, we need to point to the Launcher class. Modify build.gradle for javaFX implementation.
The program has no way of testing for errors during the runtime. Assert statements can help with this by forcing the program to terminate on bug. Added Assert statements in some classes to check for runtime error that could have been caused by programmer error.
Add Assert statements.
The Java classes have poor coding quality standards. To improve readability, it is imperative for us to fix this. Improved readability by fixing issues such as deep nested conditionals and extracting methods into helper methods amongst other tweaks.
The Java classes have poor coding quality standards. To improve readability, it is imperative for us to fix this. Improved readability by fixing issues such as deep nested conditionals and extracting methods into helper methods amongst other tweaks.
Improve Code Quality
Duplicate tasks can be added into the Duke task list. This may be a mistake by the user and may cause confusion. Added the methods isDuplicate to check for duplicates, modified the add method and added a method into UI class to implement the feature to prevent duplicate tasks when user tries to add duplicates.
Old template of Duke is for original template from upstream repo. There are alot of updates to Duke with added functionalities to support new features. Updated README for users to learn how to use the new features implemented into Duke.
BufferedReader was placed in the wrong line and may read file before file exists. This can cause the program to fail when user runs an exported jar file. Modifie lines of code to deal with this issue.
Readme was the plain template providede by the greenfield package. We should update the Readme to accurately reflect the features of Duke. Updated the Readme to reflect Duke features and how users cana use Duke.
The Storage constructor has extra try catch block for IOException handling. This is not required with the modified logic of the constructor.
Previously tracked all HTML updates. HTML files are easily generated when needed for javadocs. Removed HTML tracing via gitignore
Name of jar will be duke. Needs the jar build to be named DukeTaskBot. Changed archive build name by shadowjar plugin to be DukeTaskBot
Duker
Duker isn't like any other to-do list. You don't have to install anything on your computer!
Simply,
What features do I get with Duker?
Future features to be Added:
[ ] Make it look better for your eyes.
[ ] Make it
fasterEVEN faster[ ] Many mOre, stay tuned! 👍
If you are a Java Programmer, take a look at the source code to see how OOP this is!!
This is our main.