Skip to content
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

[Fong Yih Jie] iP #487

Open
wants to merge 59 commits into
base: master
Choose a base branch
from
Open

[Fong Yih Jie] iP #487

wants to merge 59 commits into from

Conversation

fongyj
Copy link

@fongyj fongyj commented Aug 27, 2022

DukePro

"Your mind is for having ideas, not holding them." – David Allen (source)

DukePro frees your mind of having to remember things you need to do. It's,

  • text-based
  • easy to learn
  • FAST SUPER FAST to use

All you need to do is,

  1. download it from here.
  2. double-click it.
  3. add your tasks.
  4. let it manage your tasks for you 🤨

And it is FREE!

Features:

  • Managing tasks
  • Managing deadlines (coming soon)
  • Reminders (coming soon)

If you Java programmer, you can use it to practice Java too. Here's the main method:

public class Main {
    public static void main(String[] args) {
        Duke duke = new Duke();
    }
}

Copy link

@kelvinou01 kelvinou01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, the PR was pretty good, and adhered to the style guide! There were only some minor issues that I would like you to go over. Good job!

protected TaskList tasks;
protected Parser parser;

private boolean runningDuke;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would a proposition be a clearer name for this boolean? (e.g. dukeIsRunning ?)

* @param verbose Boolean to indicate verbosity of Ui.
* @throws DukeException If command cannot be parsed or is invalid.
*/
public void parse(String command, boolean verbose) throws DukeException {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would a proposition be a clearer name for the boolean parameter? (e.g. isVerbose)?

* @throws DukeException If command cannot be parsed or is invalid.
*/
public void parseTwo(String command) throws DukeException {
String[] split = command.split(" ", 2);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would a plural name be a better name for an array?

String[] split = command.split(" ", 2);
String commandName = split[0];
int id = tasks.getSize();
if (split.length > 1){

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing major, but whitespace between ){ would be nice! May I suggest running checkStyle before submitting, it has made my life a lot easier!

* @throws DukeException If command cannot be parsed or is invalid.
*/
public void parseFind(String command) throws DukeException {
String[] split = command.split(" ", 2);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above

* Class that stores and manipulates tasks for Duke Bot.
*/
public class TaskList {
protected final ArrayList<Task> taskList;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would a name like tasks be more concise?

/**
* Marks current Task as done.
*/
public void done() {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will a verb be more apt for this method name?

/**
* Marks current Task as not done.
*/
public void undone() {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above

Copy link

@ErvinK123 ErvinK123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few nits when naming variables, nothing major, should be good to merge when changed. 👍

+ "| |_| | |_| | < __/\n"
+ "|____/ \\__,_|_|\\_\\___|\n";
private Scanner sc;
private boolean verbose;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps a more intuitive variable name here?

*/
public EventTask(String description) throws DateTimeParseException, DukeException {
super();
this.commandString = description;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps an overloaded constructor for superclass Task can be looked into

* Class that handles loading and saving of tasks for Duke Bot.
*/
public class Storage {
private FileWriter output;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps a more intuitive name could be used, that suggests the attribute is a filewriter

fongyj and others added 30 commits September 11, 2022 18:14
Included some assertions in Parser, Task and Storage class.

These assertions should help to flag out bugs in these classes during
runtime.
Improved on quality of code according to code quality guide. Removed
instances of magic number and revised bad variable naming.

This makes the code easier to read.
# Conflicts:
#	src/main/java/duke/Storage.java
Used a HashSet to store task strings for duplicate checking.

Checking for duplicate via a HashSet is faster than checking for
duplicates by iterating through an ArrayList.
Find command used to search using toString method of the tasks.
This will include (by:...) and (at:...) in the search. This will
mean that "find y" will return all deadlines and "find (" will
return all deadlines and events.
Storage used to save only after bye command. Now save after every
modification to task list.
Also updated storage file chick.txt to not store uncheck command
as it is redundant.
Duplicate check was missing in the implementation of removing the
task from duplicate check after deleting the task from task list.
Storage used to save only after bye command. Now save after every
modification to task list.
Also updated storage file chick.txt to not store uncheck command
as it is redundant.
Duplicate check was missing in the implementation of removing the
task from duplicate check after deleting the task from task list.
Changed backslash in userguide to forward slash.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants