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

[Daniel Adi.] iP #433

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

[Daniel Adi.] iP #433

wants to merge 69 commits into from

Conversation

danadi7
Copy link

@danadi7 danadi7 commented Aug 26, 2020

No description provided.

Copy link

@siqiang-ng siqiang-ng 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 code looks neat and easy to comprehend for me. Most of the classes have a comfortable length of codes for understanding. In fact, I learn more things (such as how to parse certain commands more easily) while reading your codes.

this.taskList = new ArrayList<Task>();
}

public void parseCommand(String command) throws DukeException {

Choose a reason for hiding this comment

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

The parseCommand is very clean and neat!

Copy link
Author

Choose a reason for hiding this comment

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

Thank you!

int delIndex = Integer.parseInt(temp[1]) - 1;
deleteTask(delIndex);;
} else if (command.startsWith("deadline")) {
if (command.length() <= 9){

Choose a reason for hiding this comment

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

Perhaps command.strip() could be used to remove the empty spaces after "deadline"? Was thinking how the code will work if someone input "deadline "

Copy link
Author

Choose a reason for hiding this comment

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

If someone input "deadline ", which has a length of 9, a DukeException is thrown. Nonetheless, this code is now deprecated and I have pushed a new version.

Copy link

@Anikesh99 Anikesh99 left a comment

Choose a reason for hiding this comment

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

All in all up to the standard it needs to be considering the stage at which you are at. Only small suggestions as of now like making custom exceptions.

@@ -0,0 +1,14 @@
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.

You could consider packaging the code to improve readability

Copy link
Author

Choose a reason for hiding this comment

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

Noted. Currently working on packaging.

@@ -0,0 +1,90 @@
import java.util.ArrayList;

public class TaskManager {

Choose a reason for hiding this comment

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

Probably one thing you could consider could be to extend ArrayList from taskManager to reduce the amount of code needed to be written

Copy link
Author

Choose a reason for hiding this comment

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

Do you mind elaborating further on this?

this.taskList = new ArrayList<Task>();
}

public void parseCommand(String command) throws DukeException {

Choose a reason for hiding this comment

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

The parser can possibly be placed in its own class to aid with the parsing of the dateTime objects when they start coming in

Copy link
Author

Choose a reason for hiding this comment

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

Yup, I just refactored the parser into its own class in my latest version.

}
}

public void addNewDeadline(String task) throws DukeException {

Choose a reason for hiding this comment

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

Please consider writing javadocs to aid people reading your code

Copy link
Author

Choose a reason for hiding this comment

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

Point noted. Currently in the process of adding Javadocs.

Comment on lines 7 to 10
@Override
public String toString() {
return "[T]" + super.toString();
}

Choose a reason for hiding this comment

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

Good use of overloading the toString method

Copy link
Author

Choose a reason for hiding this comment

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

Thank you!

addNewEvent(command.substring(6));
} else if (command.startsWith("todo")) {
if (command.length() <= 5){
throw new DukeException("The description of a todo cannot be empty!");

Choose a reason for hiding this comment

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

You could probably consider creating custom exceptions.

Copy link
Author

Choose a reason for hiding this comment

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

Hmm that has not crossed my mind before. Will consider it for the next version.

System.out.println("The following task has been removed: \n" + toDeleteTask);
}

public void printList() {

Choose a reason for hiding this comment

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

One thing to consider here would be that you could override your toString method to save time and code later on

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.

None yet

4 participants