-
Notifications
You must be signed in to change notification settings - Fork 364
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
[Jxleejiaxin] iP #292
base: master
Are you sure you want to change the base?
[Jxleejiaxin] iP #292
Conversation
…y them when requested
input.txt for automated text UI testing
error handling in the future.
handling of ToDo tasks to Deadline 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.
LGTM, almost. Just a few nits to fix. 😄
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.
Some methods could have been better abstracted or encapsulated. Otherwise good work!
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 and your javaDocs are well written.
I like the way you define your classes, it keeps each method short and makes them flexible to new changes.
Just take note of some coding standard violations. I have commented on a few that I spotted. Good job! 👍
* Deletes task from TaskList by index. | ||
* Prompts Ui to display deleted task followed by reminder of number of tasks in TaskList. | ||
* Prompts Storage to overwrite existing task storage file with new TaskList. |
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 detailed the descriptions are but the summary of the method may need to be written in one sentence only.
|
||
/** | ||
* Determine if Command is an instance of ExitCommand | ||
* @return true if Command subclass is ExitCommand |
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.
A minor point: In method header comments, the first sentence should start in the form Returns ..., Sends ..., Adds ... (not Return or Returnning etc.)
Maybe it would be better to change the start of the sentence to "Returns boolean value of......."
import duke.task.TaskList; | ||
import duke.ui.Ui; | ||
|
||
public class Duke { |
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.
Do you think it would be better to define the class Duke despite its functionality was already mentioned in the README?
Was there any reason why description of class was not added to Duke?
|
||
|
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 would suggest to remove excessive lines. Can try out using CheckStyle to detect coding style violations if you are interested. 😸
Add assertions
Ui class is not required when GUI is handled through JavaFx. Formatting is still required to get the desired output. The formatting of strings is encapsulated in a more suitable class. Desired output can be achieved by editing Formatter class.
Improve Code Quality
SetSourceCommand is created in Duke by inputting "source [path to file]". Users can switch between data sources and Duke will automatically load the existing tasks in the data sources.
Removes an extra try-catch statement in the long parseFromUser method. Avoids arrowhead style code.
Add different profile picture for Duke and User. Add borders to text field. Add "command: " prefix to each user input to match the assymetrical nature of the conversation by a little.
DukePro for Pros (Free [DLC)]
DukePro for Pros empowers you to remain organized when things get too hectic. It's,
All you need to do is,
And it is FREE! with FREE DLC
Features: