-
Notifications
You must be signed in to change notification settings - Fork 193
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
[Teo Ziyi Ivy] iP #182
base: master
Are you sure you want to change the base?
[Teo Ziyi Ivy] iP #182
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, I like how clean your code is and how most of your code complies with Java coding standards. Perhaps, you can consider adding some Javadoc documentation to your methods to help other users better understand your code. Great Job! 😃
src/main/java/Deadline.java
Outdated
public void printTask() { | ||
System.out.print("[D][" + getStatusIcon() + "] " + getDescription()); | ||
System.out.println("(by:" + by + ")"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps you can refactor this to a ToString() method. This way, you can print out the object directly without having to call a method:
System.out.println(deadlineObject);
src/main/java/Duke.java
Outdated
String description = line.replaceAll("/.+", ""); | ||
String by = line.replaceAll(".+/at", ""); | ||
tasks[taskCount] = new Event(description, by); | ||
taskCount = taskCount + 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps you can consider shortening this line to taskCount++
src/main/java/Duke.java
Outdated
tasks[i].printTask(); | ||
} | ||
} | ||
public static void TaskDone(int index) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps, the function name markAsDone would be a better name as it sounds more intuitive when you are accomplishing the task with this function call. In addition, you can consider Camel Casing while naming this function too.
src/main/java/Duke.java
Outdated
public class Duke { | ||
public static Task[] tasks = new Task[100]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like how you named the array in its plural form of "tasks". It sounds more intuitive
src/main/java/Duke.java
Outdated
public static void printConfused() { | ||
System.out.println("Could you say that again?"); | ||
} | ||
public static void printExit() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think adding a space after every method would make your code seem more readable and neater.
src/main/java/Duke.java
Outdated
Scanner in = new Scanner(System.in); | ||
line = in.nextLine(); | ||
//continue to run program unless types "bye" to exit program | ||
while (!line.equals("bye")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not really like the main function as it contains quite a few low level programming lines. Perhaps you could put them inside a separate method and then call this method in the main function.
src/main/java/Duke.java
Outdated
taskCount = taskCount + 1; | ||
printTaskTypeResponse(); | ||
} | ||
public static void addEvent(String line) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that you had good naming conventions for your methods, apart from your TaskDone method, mentioned by jonathanmui4.
src/main/java/Deadline.java
Outdated
@@ -0,0 +1,19 @@ | |||
public class Deadline extends Todo{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For your overall code, I think it would be more readable if you could add in comments at the top of each method/function.
src/main/java/Duke.java
Outdated
addEvent(line); | ||
} else { | ||
//error with input | ||
printConfused(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that you have included some error catching method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work Ivy! You have definitely followed 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 :D
src/main/java/duke/Deadline.java
Outdated
@@ -0,0 +1,21 @@ | |||
package duke; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great to see that you organise your types (e.g., classes) into a package for easier management!
src/main/java/duke/Duke.java
Outdated
public static void printGreeting() { | ||
System.out.println("Hello! I'm Duke\n" + "What can I do for you?"); | ||
} | ||
|
||
public static void printConfused() { | ||
System.out.println("Could you say that again?"); | ||
} | ||
|
||
public static void printExit() { | ||
System.out.println("Bye. Hope to see you again soon!"); | ||
} | ||
|
||
public static void printGotIt() { | ||
System.out.println("Got it. I've added this task:"); | ||
} | ||
|
||
public static void printTaskCount() { | ||
System.out.println("Now you have " + taskCount + " tasks in the list."); | ||
} | ||
|
||
public static void printTodoException() { | ||
System.out.println("The description of todo cannot be empty."); | ||
} | ||
|
||
public static void printDeadlineException() { | ||
System.out.println("The description of deadline cannot be empty."); | ||
} | ||
|
||
public static void printEventException() { | ||
System.out.println("The description of event cannot be empty."); | ||
} | ||
|
||
public static void printDoneException() { | ||
System.out.println("The description of done cannot be empty."); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great to see that you have created many helper print functions here! Perhaps you could also extract these final strings (also found in other parts of the code such as line 53) out as constants and put them into a new Message
class later on?
src/main/java/duke/Deadline.java
Outdated
@@ -0,0 +1,21 @@ | |||
package duke; | |||
|
|||
public class Deadline extends Todo{ |
There was a problem hiding this comment.
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. Similar styles also happen in line 3 of DeadlineException
class.
src/main/java/duke/Deadline.java
Outdated
} | ||
|
||
public void printTask() { | ||
System.out.print("[D][" + getStatusIcon() + "] " + getDescription()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to https://nus-cs2113-ay2122s1.github.io/website/se-book-adapted/chapters/codeQuality.html#avoid-magic-numbers, perhaps you could spend some time trying to avoid all sorts of magic strings (there are also found elsewhere such E
in Event class) to further improve readability?
src/main/java/duke/Duke.java
Outdated
} | ||
|
||
public static void printTaskTypeResponse() { | ||
//printing different responses depending if its duke.Todo/duke.Deadline/duke.Event |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could consider removing this comment and use 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. The same applies to line 85.
src/main/java/duke/Task.java
Outdated
// mark done task with X | ||
return (isDone ? "X" : " "); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like how your comment is indented relative to its position in the code. Good job!
src/main/java/duke/Deadline.java
Outdated
public void setBy(String by) { | ||
this.by = by; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Precise setter name, good job!
src/main/java/duke/Task.java
Outdated
|
||
public class Task { | ||
protected String description; | ||
protected boolean isDone; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like how your use the is
prefix here to make your boolean attribute sound like valid booleans.
src/main/java/duke/Duke.java
Outdated
} | ||
} | ||
|
||
public static void taskDone(String line) throws DoneException { |
There was a problem hiding this comment.
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.
src/main/java/duke/Duke.java
Outdated
} | ||
|
||
public static void readInput(String line) { | ||
String[] splitLine = line.split(" ", 2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps you could also spend some time to avoid having magic numbers as well.
Added Levels-0-3 and A-CodingStandard