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

[Rachel Keh] iP #190

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

Conversation

rachelkeh
Copy link

No description provided.

adding in Task class only now even though it was introduced in the level 1 increment because I forgot to let Git track it
…tions and invoke from main.

Change taskDetails to description in Task class to flow with the rest of the code and avoid unnecessary confusion.
Change a few magic numbers into constants in TaskManager class.
Copy link

@jiexiong-zeng jiexiong-zeng left a comment

Choose a reason for hiding this comment

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

Looks great 👍

Scanner in = new Scanner(System.in);
line = in.nextLine();

TaskManager t1 = new TaskManager();

Choose a reason for hiding this comment

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

Can you consider a better variable name?

String[] details = keywords[1].split(" /");
tasks[Task.getNumberOfTasks()] = new Event(details[0], details[1].substring(DESCRIPTION_AFTER_BY_OR_AT));
}
System.out.println(" _____________________________________________________________");

Choose a reason for hiding this comment

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

Can you consider creating a println function to use instead?

@@ -0,0 +1,41 @@
public class TaskManager {

Choose a reason for hiding this comment

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

I like that the TaskManager is seperate from Duke. Makes the code more readable.


public void addTask(String line) {
String[] keywords = line.split(" ", 2);
if (keywords[INDEX_OF_KEYWORD_OF_TASK].equalsIgnoreCase("todo")) {

Choose a reason for hiding this comment

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

You can consider making "todo", "deadline", "event" String constants

Copy link

@Samuel787 Samuel787 left a comment

Choose a reason for hiding this comment

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

I've added some suggestions to improve your code quality. Coding style looks good to me :)

@@ -0,0 +1,20 @@
package duke.task;

public class Deadline extends Task {

Choose a reason for hiding this comment

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

Good job! You made use of OOP to make Deadline a subclass of Task :)

if (words[0].equals("T")) {
tasks.add(new Todo(words[2]));
if (words[1].equals("true")) {
tasks.get(Task.getNumberOfTasks() - 1).setDone(); //else leave isDone as false

Choose a reason for hiding this comment

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

By splitting this line into two lines like this:

int lastTaskIndex = Task.getNumberOfTasks() - 1;
tasks.get(lastTaskIndex).setDone()

helps to improve code readability

} else if (words[0].equals("D")) {
tasks.add(new Deadline(words[2], words[3]));
if (words[1].equals("true")) {
tasks.get(Task.getNumberOfTasks() - 1).setDone(); //else leave isDone as false

Choose a reason for hiding this comment

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

The code is kinda simple and self-explanatory. Hence, I feel the comment is rather redundant. You may consider removing it :)

if (words[1].equals("true")) {
tasks.get(Task.getNumberOfTasks() - 1).setDone(); //else leave isDone as false
}
} else { //the only type of Task left is Event

Choose a reason for hiding this comment

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

You may want to write the comment in a new line to improve code readability :)

printIncorrectInput(isEmptyDescription, keywords[INDEX_OF_KEYWORD_OF_TASK]);
}
}
if (hasNoException) {

Choose a reason for hiding this comment

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

Instead of using a boolean to see if an exception has occurred, you may consider throwing the exception back to the caller method and let the calling method handle the exception. In that way, you can confidently printAddedTasked() and updateFile(). Because if an exception had occurred, an exception will be thrown from the method and this part of the code will not be reached. Just a suggestion :)


public void markAsDone(String index) {
try {
int indexInteger = Integer.parseInt(index);

Choose a reason for hiding this comment

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

Since you are calling this variable index, you can do the -1 in this line itself
int indexInteger = Integer.parseInt(index) - 1;


public void deleteTask(String index) {
try {
int indexInteger = Integer.parseInt(index);

Choose a reason for hiding this comment

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

Same suggestion. If you do -1 in this line, you don't have to do it everywhere else in this method.
int indexInteger = Integer.parseInt(index) - 1;

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