Skip to content

[Jared Lim] iP#276

Open
jared98lyj wants to merge 40 commits into
nus-cs2103-AY2021S2:masterfrom
jared98lyj:master
Open

[Jared Lim] iP#276
jared98lyj wants to merge 40 commits into
nus-cs2103-AY2021S2:masterfrom
jared98lyj:master

Conversation

@jared98lyj
Copy link
Copy Markdown

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
Comment thread src/main/java/Duke.java Outdated

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

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

Comment thread src/main/java/Event.java Outdated
}

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

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.

Comment thread src/main/java/Duke.java Outdated
}

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

Choose a reason for hiding this comment

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

Minor typo for "list"

Comment thread src/main/java/Task.java
* @param description Description of method
* @param eventDate Boolean flag indicating whether task is done
*/
public Task(String description, String eventDate) {
Copy link
Copy Markdown

@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
Copy Markdown

@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 :)

Comment thread src/main/java/Duke.java
/**
* Driver class that takes in input and performs certain functions based on input.
*/
public class Duke {
Copy link
Copy Markdown

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?

Comment thread src/main/java/Duke.java Outdated
} else if (command.equals("list")) {
enumerateTasks();
} else if (command.startsWith("done")) {
String[] delString = command.split("\\s+");
Copy link
Copy Markdown

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?

Comment thread src/main/java/Event.java Outdated
}

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

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 thread src/main/java/Task.java Outdated
Comment on lines +8 to +10
low,
average,
high
Copy link
Copy Markdown

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"

Comment thread src/main/java/Task.java Outdated
}

/**
* Overriden method to mark a task as done.
Copy link
Copy Markdown

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