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

[Jared Lim] iP #276

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

Conversation

jared98lyj
Copy link

No description provided.

Added ArrayList to store input.
Improved layout of code.
Added children classes for task to give more granularity to the different types of tasks.
Added try-catch blocks to flag errors, and prevent possible incorrect input from created unwanted stops/breakages in the program.
Added the removeTask method for deletion
@jared98lyj jared98lyj changed the title Updated till level 6. [Jared Lim] IP Feb 6, 2021
@jared98lyj jared98lyj changed the title [Jared Lim] IP [Jared Lim] iP Feb 6, 2021

/**
* Main class to take in user input.
* @param args Filler

Choose a reason for hiding this comment

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

Perhaps a more meaningful name for @param could be possibly replaced here

}

/**
* Overriden constructor to account for deadline and events that have timestamps.

Choose a reason for hiding this comment

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

Not too sure if "Overriden" is the appropriate term here. I think "Overloaded constructor" may be more appropriate.

}

/**
* Enumerates all tasks in the lis using 1-based indexing.

Choose a reason for hiding this comment

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

Minor typo for "list"

* @param description Description of method
* @param eventDate Boolean flag indicating whether task is done
*/
public Task(String description, String eventDate) {
Copy link

@justgnohUG justgnohUG Feb 6, 2021

Choose a reason for hiding this comment

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

Might be a good idea to add "@OverRide" tag if this method is truly overriden

Added I/O utility.
Autocreate data directory if it does not exist.
Copy link

@wei-yutong wei-yutong left a comment

Choose a reason for hiding this comment

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

your code is generally very clean and easy to read! i liked how your method names were clear and how you separated long lines accurately :)


/**
* Driver class that takes in input and performs certain functions based on input.
*/
public class Duke {

Choose a reason for hiding this comment

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

this class is rather long, perhaps you could consider abstracting the methods out?

} else if (command.equals("list")) {
enumerateTasks();
} else if (command.startsWith("done")) {
String[] delString = command.split("\\s+");

Choose a reason for hiding this comment

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

delString is not a very intuitive variable name, maybe you could make it more specific?

}

/**
* Overriden constructor to account for deadline and events that have timestamps.

Choose a reason for hiding this comment

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

i think the term you meant to use might be "overloaded" instead

Comment on lines 8 to 10
low,
average,
high

Choose a reason for hiding this comment

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

I think that constants in enum should be in all uppercase, eg "LOW", "AVERAGE" and "HIGH"

}

/**
* Overriden method to mark a task as done.

Choose a reason for hiding this comment

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

Did you mean "Overloaded" here as well? I've noticed it in a few other places. Otherwise, as the other reviewer mentioned, overridden methods should tagged with "@OverRide"

Added date time formatter across all task classes. Created a ParseDate class to handle date formatting.
Add-gradle-support
Included J-unit tests under dependencies in build.gradle file
Added gradle wrapper files as well
Added a find function uitlity to filter tasks by keyword
Refactored a bulk of the code from Duke.java into four classes: Parser, Storage, TaskList, and Ui.
Modified Duke.java to accomodate these changes as well.
Included the gradle wrapper and the associated dependencies for the Junit test.
Added two JUnit Test: One to test the loading of the stored text file, the other to test marking as done utility. Both under src/test/java.
Applied changes to all other dependent classes as well.
Added GUI and modified the corresponding methods to return a string (rather than stdout) to feed into the user input functions, which are returned and displayed in the dialogue box in GUI.
…checks for non empty objects, and list to avoid unchecked null pointers which can crash the program.
…s as much as possible by abstracting it away into the Task classes.
…ption to specify type of search: partial case sensitive, partial non- case sensitive, full exact match.
GUI echoes user commands, returns output by bot.
…method body. Prevented as much as possible the use of magic strings when it comes to important kinds of data. Avoided direct usage of list indexing in Duke method by

abstracting to the TaskList method.
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