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

[Ng Si Qiang] iP #252

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

Conversation

siqiang-ng
Copy link

No description provided.

/**
* Contains all the saved tasks in a List form
*/
public class TaskList {

Choose a reason for hiding this comment

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

Could consider if TaskList should be placed inside the task package or along side the duckie class

Comment on lines +7 to +12
public class DuckieTest {
@Test
public void dummyTest() {
assertEquals(2, 2);
}
}

Choose a reason for hiding this comment

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

Could consider getting rid of this test class since it actually is just a dummy test that doesn't really test any of the features in your actual code


import duckie.exception.DuckieException;

public class TaskListTest {

Choose a reason for hiding this comment

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

Very comprehensive testing of tasklist. 👍

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 very good coding. Checkstyle seems to be alright and the code mostly adheres to the OOP principles. No glaring lapses found so only minor suggestions have been made. Keep up the great work :-).

Copy link

@ya0-yuan ya0-yuan left a comment

Choose a reason for hiding this comment

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

Overall, I fell that your code is very concise and readable. Javadoc is complete for almost all classes and public methods and most of the coding standards are followed. It looks good to go for merging!

import duckie.task.Deadline;
import duckie.task.Event;
import duckie.task.Task;
import duckie.task.Todo;
Copy link

Choose a reason for hiding this comment

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

I like how you arrange your import statements tidy and in order!

if (!duckieFile.exists()) {
File dir = new File(duckieFile.getParent());
if (!dir.exists()) {
if (dir.mkdirs() && duckieFile.createNewFile()) {
Copy link

Choose a reason for hiding this comment

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

Perhaps you may consider avoid deep nesting (up to 3 levels of indentation) here to make the code more readable.

* Return the type of a Task
* @return null
*/
public String getType() {
Copy link

Choose a reason for hiding this comment

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

Perhaps you may consider change Task to an abstract class so that the implementation of getType(), getDate() can added later in child classes, instead of returning null value.

Comment on lines 9 to 13





Copy link

Choose a reason for hiding this comment

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

Perhaps you can consider get rid of so many empty lines.

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