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

[Ho Jing Yang Daniel] ip #60

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

Conversation

Bencotti
Copy link

No description provided.

Copy link

@Zhilin-Huang Zhilin-Huang left a comment

Choose a reason for hiding this comment

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

Consider adding *.class in your .gitignore file so that you do not commit the binary .class files :)

System.out.println( "["+ currentTask.getTaskType() + "][" + currentTask.getStatusIcon() + "] " + currentTask.getDescription() + "\n");
}
} else {
System.out.println("Error: No such index in use\n");

Choose a reason for hiding this comment

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

Nice to take corner cases into account by specifying what to do when the task index is out of bound!

src/main/java/Task.java Outdated Show resolved Hide resolved
text-ui-test/ACTUAL.TXT Outdated Show resolved Hide resolved
public void printListDetails(int count) {
System.out.println("["+ getTaskType() + "][" + super.getStatusIcon() + "] " + count + ". " + super.description + " (by: " + date + ")");
}

Choose a reason for hiding this comment

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

Possible redundant blank lines?

public static final int LENGTH_DEADLINE = 9;
public static final int LENGTH_EVENT = 6;
public static final int LENGTH_TODO = 5;
public static final int SIZE_DONE_COMMAND = 2;

Choose a reason for hiding this comment

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

Good job factoring out the constants!

return null;
}

public void printAddDetails(int taskCounter) {

Choose a reason for hiding this comment

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

Consider refactoring the printAddDetails() function so that you don't need to duplicate part of the print statement in the child classes


public static void completeTask(ArrayList<Task> tasks, int taskIndex) {
taskIndex--; // index starts from 0, unlike listing number
if ( (taskIndex < tasks.size()) || (taskIndex > 0)) { // check if out of bounce

Choose a reason for hiding this comment

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

Minor typo: bounce -> bound

System.out.println("");
}

public static void printHelp() {

Choose a reason for hiding this comment

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

Great idea to provide a guide to help users understand how to use your bot :D


@Override
public String getTaskType(){
return "T";

Choose a reason for hiding this comment

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

Consider making T a constant

Copy link
Author

Choose a reason for hiding this comment

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

Good suggestion!

Copy link

@JTWang2000 JTWang2000 left a comment

Choose a reason for hiding this comment

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

You've done a really good job for coding standard. Also, I really appreciate that you add additional features into your project to make it have your own design. There is only one small suggestion that you can do further abstraction to make the code cleaner. Overall, it is really a good work.

Review together with @yuchenlichuck

} else {
currentTask.markAsDone();
System.out.println("Nice! I've marked this task as done:");
System.out.println( "["+ currentTask.getTaskType() + "][" + currentTask.getStatusIcon() + "] " + currentTask.getDescription() + "\n");

Choose a reason for hiding this comment

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

This line is a little bit too long. Maybe you can split this line into two rows.

System.out.println("Commands: ");
System.out.println("List: lists all recorded tasks \nusage: list\n");
System.out.println("Done: mark task as completed \nusage: done <task number>\n");
System.out.println("Todo: Tasks without date/time \nUsage: todo <task> \n(Avoid using other keywords as the first word)\n");

Choose a reason for hiding this comment

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

You can consider to make this string into constant if you are going to use it for mutiple times.This can improve the readibility of the code.

}

public void printAddDetails(int taskCounter) {
System.out.println("The following task has been added:\n[" + getTaskType() +"][" + getStatusIcon() + "] " + getDescription());

Choose a reason for hiding this comment

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

Please pay attention to line length. Try to keep line length shorter than 110 characters (soft limit). If the line exceeds the limit, use line wrapping at appropriate places of the line.


public static void completeTask(ArrayList<Task> tasks, int taskIndex) {
taskIndex--; // index starts from 0, unlike listing number
if ( (taskIndex < tasks.size()) || (taskIndex > 0)) { // check if out of bounce

Choose a reason for hiding this comment

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

There is a coding standard violation that between the first two barcekets, there should be no space.


System.out.println("\n" + logo + "\nYour Lifestyle Scheduling Assistant\n");
System.out.println("type \"help\" for list of commands");
System.out.println("____________________________________________________________\n");

Choose a reason for hiding this comment

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

If we print this long line mutiple times, we can make it a constant.

} catch (Exception e) {
System.out.println("Please input a valid number\n");
}
break;

Choose a reason for hiding this comment

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

Inside the case "done": we can add a new function which extract these codes. It is not a coding standard violation, i am just commmenting to make your switch cases much cleaner.

@Zhilin-Huang
Copy link

Reviewed with @sliu107

# Conflicts:
#	Duke/data.txt
#	src/main/java/Command/AddCommand.java
#	src/main/java/Duke/Deadline.java
#	src/main/java/Duke/Duke.java
#	src/main/java/Duke/Event.java
#	src/main/java/Duke/Task.java
#	src/main/java/Task/TaskList.java
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