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

edward wang ip #194

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

Conversation

EdwardZYWang
Copy link

No description provided.

Comment on lines 28 to 47
} else if (input.contains("todo")) {
Task whatToDo = toDoMethod(input);
System.out.println(" Got it. I've added this task:\n" + " " + whatToDo + "\n"
+ " Now you have " + inputCount + " tasks in the list.");

input = in.nextLine();

} else if (input.contains("deadline")) {
Task whatDeadline = deadlineMethod(input);
System.out.println(" Got it. I've added this task:\n" + " " + whatDeadline + "\n"
+ " Now you have " + inputCount + " tasks in the list.");

input = in.nextLine();

} else if (input.contains("event")) {
Task whatEvent = eventMethod(input);
System.out.println(" Got it. I've added this task:\n" + " " + whatEvent + "\n"
+ " Now you have " + inputCount + " tasks in the list.");

input = in.nextLine();

Choose a reason for hiding this comment

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

I suggest you put all the System.out.println to separated methods, to make main class clear and clean

}

public static Task toDoMethod(String input) {
String toDoDescription = input.substring(4).trim();

Choose a reason for hiding this comment

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

We should avoid "magic numbers" like 4 here, you can define a CONST to replace 4

Comment on lines 93 to 105
private static String taskDone(Scanner in, Task task) {
String statusOfTask;
String input;
System.out.println(" Nice! I've marked this task as done:" + System.lineSeparator());
System.out.println(" " + task + " has been updated to -->");

task.markAsDone(); //mark x in [ ]
statusOfTask = task.getStatusIcon();

System.out.println(" " + task);
input = in.nextLine();
return input;
}

Choose a reason for hiding this comment

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

For naming of a method, we usually start with a verb

Comment on lines 117 to 139
public static Task deadlineMethod(String input) {
String[] deadlineSplitter = input.substring(9).split(" /by ");
String deadlineDescription = deadlineSplitter[0]; //before /by
String deadlineBy = deadlineSplitter[1]; // after /by

Task description = new Deadline(deadlineDescription, deadlineBy);
tasks[inputCount] = description;
inputCount++;

return description;
}

public static Task eventMethod(String input) {
String[] eventSplitter = input.substring(6).split(" /at ");
String eventDescription = eventSplitter[0]; //before /at
String eventAt = eventSplitter[1]; // after /at

Task description = new Event(eventDescription, eventAt);
tasks[inputCount] = description;
inputCount++;

return description;
}

Choose a reason for hiding this comment

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

same as above, the naming convention of mothods

}
}
System.out.println(" Bye! Hope to see you again soon!");

Choose a reason for hiding this comment

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

The whole while and if else structure is quite nice and clear!

@@ -0,0 +1,32 @@
public class Task {
Copy link

Choose a reason for hiding this comment

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

Would be good to add descriptive header comments for all public classes/methods

+ " Now you have " + inputCount + " tasks in the list.");

input = in.nextLine();

Copy link

Choose a reason for hiding this comment

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

Not sure if it is intended but there is some unnecessary whitespace before closing braces of if-else statements

String input;
System.out.println(" Here are the tasks in your list:");

for (int outputCount = 0; outputCount < inputCount; outputCount++) {
Copy link

Choose a reason for hiding this comment

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

Can consider using a shorter name for the "outputCount" variable as it is located within a very small scope

return input;
}

private static String taskDone(Scanner in, Task task) {
Copy link

Choose a reason for hiding this comment

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

Would be clearer if this is named as a verb, like setTaskDone()

Copy link

@zikunz zikunz left a comment

Choose a reason for hiding this comment

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

Great work Edward! You have definitely been following various coding guidelines and your Duke is so awesome! 💯

Overall, this version of code is very easy to understand and is very readable except for very few potential and minor coding standard and quality-related violations. Keep up the good work :)

Comment on lines 42 to 43
System.out.println(" Got it. I've added this task:\n" + " " + whatDeadline + "\n"
+ " Now you have " + inputCount + " tasks in the list.");
Copy link

Choose a reason for hiding this comment

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

Perhaps you could consider having a function for printing and name it as displayUserIntroText() or the like.

Comment on lines 16 to 24
while (!(input.equals("bye"))) {

if (input.equals("list")) {
input = printList(in);

} else if (input.contains("done")) {
input = testTaskDone(input, in);

} else if (input.contains("todo")) {
Copy link

Choose a reason for hiding this comment

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

Storing "bye" / "list" / ... as constants above and naming them as
private final String INPUT_BYE = "bye" or the like could be more readable.

Comment on lines 29 to 34
System.out.println(" Got it. I've added this task:\n" + " " + whatToDo + "\n"
+ " Now you have " + inputCount + " tasks in the list.");

input = in.nextLine();
} catch (DukeExceptions e3) {
System.out.println("☹ OOPS!!! The description of a todo cannot be empty.");
Copy link

Choose a reason for hiding this comment

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

Perhaps you could consider to extract these final strings (also found in other parts of the code such as line 42) out as constants and put them into a new Message class later on?

return input;
}

public static Task toDoMethod(String input) {
Copy link

@zikunz zikunz Sep 14, 2021

Choose a reason for hiding this comment

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

Perhaps the method name to be reprhased so that it is more descriptive about what it does? Also, from this week onwards, consider using JavaDoc comments for non-private classes / methods and non-trivial private methods in an appropriate way, as per https://nus-cs2113-ay2122s1.github.io/website/se-book-adapted/chapters/documentation.html#javadoc.


public static Task deadlineMethod(String input) {

String[] deadlineSplitter = input.substring(9).split(" /by ");
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 avoiding magic numbers like 9 here. The same applies to line 145, 157 etc.

@@ -0,0 +1,2 @@
public class DukeExceptions extends Exception{
Copy link

@zikunz zikunz Sep 14, 2021

Choose a reason for hiding this comment

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

Perhaps you could leave a space here before { for layout consistency.

Comment on lines +3 to +9
public Todo(String description, String tasksToDo) {
super(description);
}

public Todo(String description) {
super(description);
}
Copy link

Choose a reason for hiding this comment

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

Great use of overloading the constructor!


@Override
public String toString() {
return "[T]" + "[" + getStatusIcon() + "]" + " " + description;
Copy link

Choose a reason for hiding this comment

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

Perhaps you could use a named constant to avoid magic strings here as well.

Comment on lines 48 to 60
input = in.nextLine();
} catch (ArrayIndexOutOfBoundsException e2) {
System.out.println("you have typed in = " + input + "\n☹ OOPS!!! where is your /by my friend? ");
input = in.nextLine();
}

} else if (input.contains("event")) {
try {
Task whatEvent = eventMethod(input);
System.out.println(" Got it. I've added this task:\n" + " " + whatEvent + "\n"
+ " Now you have " + inputCount + " tasks in the list.");

input = in.nextLine();
Copy link

Choose a reason for hiding this comment

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

It looks like input = in.nextLine(); is called in many else if blocks, perhaps you could consider calling it only once to avoid repetitive code.


}

public static Task deadlineMethod(String input) {
Copy link

Choose a reason for hiding this comment

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

Perhaps you could rephrase the method name here so that it starts with a verb according to https://se-education.org/guides/conventions/java/index.html#naming.

EdwardZYWang added 8 commits September 16, 2021 17:47
# Conflicts:
#	src/main/java/Duke.java
add constants to represent commands
change method names according to SE conventions
add relative path for duke.txt
Give users a way to find a task by searching for a keyword.
Teach Duke how to understand dates and times.
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