Skip to content

[Minyue] iP - #193

Open
astralum wants to merge 38 commits into
nus-cs2113-AY2122S1:masterfrom
astralum:master
Open

[Minyue] iP#193
astralum wants to merge 38 commits into
nus-cs2113-AY2122S1:masterfrom
astralum:master

Conversation

@astralum

@astralum astralum commented Sep 1, 2021

Copy link
Copy Markdown

No description provided.

Comment thread src/main/java/Duke.java Outdated
Comment on lines +59 to +57
tasks[id].setIsDone();
tasks[id].setDone();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

nice :D

Move task handling logic into a new class

@yuejunfeng0909 yuejunfeng0909 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You may want to consider add the methods to the individual classes instead of using a separate function to handle all the methods

Comment thread src/main/java/Duke.java Outdated
public static void main(String[] args) {

public static void sayHello() {
String logo = " ____ _ \n"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

since logo should be a constant, can change the variable name to uppercase and set it as a constant

Comment thread src/main/java/TaskHandler.java Outdated
protected Task[] tasks;

public TaskHandler() {
this.tasks = new Task[100];

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

you may want to use a constant MAX_TASKS instead of putting a random number here

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

public String handleTasks(String line) {
String lc = line.toLowerCase();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

try not to use acronyms in the codes

Handle user input errors using exceptions

Tidy Formatter code
Place into a package duke
Update implementation of TaskHandler to use ArrayList instead of array
Add ability to add, edit and clear tasks stored in the external file
Add ability to add, edit and clear tasks stored in the external file

@shivin9 shivin9 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nice code overal. Formatting can be imporved though

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

protected String at;

public Event(String description, String at) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Can have a more descriptive name than 'at'

return OUTPUT_START;
}

public static void printInputStart() {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You can have a small description before functions...

Comment thread src/main/java/duke/TaskHandler.java Outdated
}
}

private void openFile() {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Comments before functions would help...

Comment thread src/main/java/duke/TaskHandler.java Outdated
Todo newTodo = new Todo(description);
tasks.add(newTodo);
appendLinetoFileData(newTodo.toString());
return returnAddTaskSuccess() + newTodo.toString();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You can break this line into two according to me

Comment thread src/main/java/duke/TaskHandler.java Outdated
return addDeadline(line);
}
} else if (inputIsEvent(lc)) {
if (!eventContainsAt(lc)) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Try to avoid double nesting if possible...

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

protected String description;
protected boolean isDone;

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 line can be removed IMHO

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.

4 participants