-
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
[Cheak Han Wei] iP #154
base: master
Are you sure you want to change the base?
[Cheak Han Wei] iP #154
Conversation
Updated A-TextUiTesting to test for errors
Updated A-TextUiTesting to test for Delete
Tasks are not saved and only stored in temporary variables. Users start the program with clean data. We will save the data in the hard disk and load the data when Duke starts up.
Dates were stored as Strings. Not very meaningful to do so. Parsing dates and storing them as LocalTimeDate Objects allow us to perform more meaningful operations that deal with time and also to standardise users' input.
Code was merged from branch-Level-8 to master branch. Errors were thrown when Duke was ran. Refactored Level-8.
Made the program more OOP by extracting Ui, Parser, TaskList and the Commands classes.
Working directory is very cluttered. Put the files into different packages.
Add JUnit for testing code. Added methods to test methods in AddCommand, DeleteCommand and ExitCommand classes.
Added JavaDocs to Commands classes and Duke. JavaDocs helps external users to understand our code better.
Tweak the code to comply with the SE-EDU coding standard.
Give users a way to find a task by searching for a keyword.
Missed out the FindCommand class in the previous commit.
Automate project builds using Gradle. Gradle can run Unit tests, as well as run the Duke program.
To detect coding style violations.
Simple GUI implemented for Duke.
Implemented GUI using fxml.
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, almost. I like how you structure your classes very neatly so that it is easy to understand as someone new to your codes :) Good job 👍
|
||
private void printTotalNumberOfTasks() { | ||
int numTasks = tasks.size(); | ||
if (numTasks < 2) { |
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 there a better way to structure these to extract common code and reduce duplicates?
src/main/java/Duke.java
Outdated
*/ | ||
public void run() { | ||
ui.showWelcome(); | ||
boolean isExit = 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.
Would it be better to change "isExit" to "hasExit" to make it more natural?
* | ||
* @return true or false | ||
*/ | ||
public abstract boolean isExit(); |
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 why this must be a method instead of a boolean attribute?
* @param storage storage storage that handles data storage | ||
*/ | ||
@Override | ||
public void execute(TaskList tasks, Ui ui, Storage storage) { |
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 it be better to name it more specifically, e.g. "listOutTasks" instead of "execute" ?
To satisfy the iP Progress Dashboard. Something went wrong with the previous A-Gradle tagging.
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 the way you structure your classes! . Overall LGTM:)
src/main/java/MainWindow.java
Outdated
* the dialog container. Clears the user input after processing. | ||
*/ | ||
@FXML | ||
private void handleUserInput() { |
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 you might want add a condition to check if the user wants to exit, before using Platform.exit() to terminate the javaFx window.
switch (commandWord) { | ||
case "todo": | ||
return taskList.addTodo(taskName, storage); | ||
|
||
case "deadline": | ||
return taskList.addDeadline(taskName, storage); | ||
|
||
case "event": | ||
return taskList.addEvent(taskName, storage); | ||
|
||
default: | ||
return ""; |
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 job on the switch indentation!
Wrote assumptions that should hold at various points in the code.
Examine code and refactor to improve code quality.
Add A-Assertions: Use Assertions
Add A-CodeQuality
Use Continuous Integration.
Futher improvements on code quality.
Allows tagging of tasks.
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.
Hey Buddy! Your code looks clean and all things seem to be well abstracted.. Kudos
No description provided.