-
Notifications
You must be signed in to change notification settings - Fork 78
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
[Lim Yuh Ching] iP #79
base: master
Are you sure you want to change the base?
Conversation
ToDos, Events, Deadlines are created as a subclass of Task. Moved addToTask and printTask from Task class to Main program.
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.
Overall code is easy to read, little to no standard coding violations. Everything looks good to me
src/main/java/Deadlines.java
Outdated
@@ -0,0 +1,18 @@ | |||
public class Deadlines extends Task { | |||
String 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.
Good following of spacing requirements
src/main/java/Duke.java
Outdated
private static int taskCount = 0; | ||
|
||
public static void addToTasks(String description, Type type) { | ||
switch (type) { |
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.
Good and clear use of switch statements
src/main/java/Duke.java
Outdated
System.out.println("\tNow you have " + taskCount + " task(s) in the list."); | ||
} | ||
public static void printTasks() { | ||
if (taskCount == 0) { |
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.
Good use of if-statements, no nesting errors
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.
Missed out 1 comment previously, everything looks good to me
src/main/java/Deadlines.java
Outdated
super(description); | ||
this.by = by; | ||
} | ||
public String getBy() { |
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.
Good following of naming of method conventions
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 structure your code! I just made some suggestions to the variable name to make your code more clear to understand
src/main/java/Duke.java
Outdated
Scanner scanner = new Scanner(System.in); | ||
String userInput = scanner.nextLine(); | ||
String[] substr = userInput.split("\\s+"); | ||
switch (substr[0]) { |
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.
command handling is well-organised using a switch case which makes it easier to troubleshoot.
src/main/java/Duke.java
Outdated
} | ||
} | ||
} | ||
public static void 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.
Perhaps you could use a more clear method name such as printByeMessage?
public static void bye() { | |
public static void printByeMessage() { |
src/main/java/Duke.java
Outdated
tasks[taskCount] = new ToDos(description); | ||
break; | ||
case DEADLINE: | ||
int i = description.indexOf("/"); |
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.
Maybe you can use a better variable name for the index like indexAfterBy? so that it is more clearer when you revisit the code
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.
Looks good and neat!
src/main/java/Duke.java
Outdated
case "bye": | ||
bye(); | ||
break; | ||
case "list": | ||
printTasks(); | ||
break; | ||
case "mark": | ||
markAsDone(Integer.parseInt(substr[1])); | ||
break; | ||
case "unmark": | ||
markAsUndone(Integer.parseInt(substr[1])); | ||
break; | ||
case "todo": | ||
addToTasks(userInput.substring(5), Type.TODO); | ||
break; | ||
case "deadline": | ||
addToTasks(userInput.substring(9), Type.DEADLINE); | ||
break; | ||
case "event": | ||
addToTasks(userInput.substring(6), Type.EVENT); | ||
break; | ||
} |
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 you use switch statements for the various instructions. This makes your code very neat.
this.description = description; | ||
} | ||
|
||
public 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.
Perhaps the method could sound more verb-like?
public boolean isDone() { | |
public boolean checkIsDone() { |
public enum Type { | ||
TODO, DEADLINE, 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.
Good use of enum for the various tasks!
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.
Overall, your code follows the standards decently. However, do look out for things like naming conventions and magic types which can be avoided and will help to improve your code.
src/main/java/Duke.java
Outdated
+ "|____/ \\__,_|_|\\_\\___|\n"; | ||
System.out.println("Hello from\n" + logo); | ||
// Greetings | ||
String intro = "~~~~~~~~~~~~~~~~~~~\n" |
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 this could be made a constant.
src/main/java/Duke.java
Outdated
int j = description.indexOf("/"); | ||
int k = description.indexOf("/", j+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.
For the following methods, you could use more descriptive variables rather than single letter variables. This would be more in line with the naming conventions required.
The name change suggested by your classmate would be a good example of how to name the variable instead!
This reverts commit 2e8d61b.
# Conflicts: # src/main/java/Duke.java
Create TaskList, Parser, Ui and Storage to split the code in Duke into different objects
Add Javadoc to most of the class
No description provided.