-
Notifications
You must be signed in to change notification settings - Fork 200
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
[Tze Loong] iP #213
base: master
Are you sure you want to change the base?
[Tze Loong] iP #213
Conversation
public class Duke { | ||
public static void main(String[] args) { |
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.
Consistent 4 spaces indent throughout the code, good job
src/main/java/Duke.java
Outdated
greet_user(); | ||
int counter = 0; | ||
|
||
Tasks[] list_of_tasks = new Tasks[101]; // Following the assumption that there will be no more than 100 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.
Perhaps you try using camelcase when naming variables. Eg. listOfTasks
src/main/java/Tasks.java
Outdated
@@ -0,0 +1,23 @@ | |||
public class 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.
Perhaps you could name this class as "Task" (singular) instead as it refers to one task
src/main/java/Duke.java
Outdated
System.out.println("How can i help u? \n"); | ||
} | ||
|
||
public static void echo() { |
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 job for using a verb to name this 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.
In general, the code is quite clear, however, there is a lot of potential for clearer and more concise code.
src/main/java/Duke.java
Outdated
if ("list".equals(input)) { | ||
System.out.println("Here are the tasks in your list: "); | ||
for (int i = 0; i < counter; i++) { | ||
System.out.println( | ||
i + 1 + "." + " " + list_of_tasks[i].getStatusIcon() + " " + list_of_tasks[i].description); | ||
} | ||
} else if (input.length() >= 4 && input.substring(0, 4).equals("mark")) { | ||
Integer index = Integer.valueOf(input.substring(5, input.length())); | ||
list_of_tasks[index - 1].markAsDone(); | ||
System.out.println("Nice! This task is completed"); | ||
System.out | ||
.println(list_of_tasks[index - 1].getStatusIcon() + " " + list_of_tasks[index - 1].description); | ||
} else if (input.length() >= 6 && input.substring(0, 6).equals("unmark")) { | ||
Integer index = Integer.valueOf(input.substring(7, input.length())); | ||
list_of_tasks[index - 1].markAsUnDone(); | ||
System.out.println("Ok, This task is still not complete"); | ||
System.out | ||
.println(list_of_tasks[index - 1].getStatusIcon() + " " + list_of_tasks[index - 1].description); | ||
} else { | ||
System.out.println("added: " + input); | ||
list_of_tasks[counter].description = input; | ||
list_of_tasks[counter].isDone = false; | ||
counter++; | ||
} | ||
scan = new Scanner(System.in); | ||
input = scan.nextLine(); |
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.
Since you abstracted the [greet_user()] function, it may be better if you maintain Single Level of Abstraction Principle (SLAP), and also extract this portion as a separate function
src/main/java/Duke.java
Outdated
greet_user(); | ||
int counter = 0; | ||
|
||
Tasks[] list_of_tasks = new Tasks[101]; // Following the assumption that there will be no more than 100 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.
It may be better to avoid using a magic number (100) and instead define it at the top of the file as MAX_NUMBER_OF_TASKS
src/main/java/Duke.java
Outdated
} | ||
|
||
public static void main(String[] args) { | ||
greet_user(); |
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 job on extracting this part as a separate function
src/main/java/Duke.java
Outdated
if ("list".equals(input)) { | ||
System.out.println("Here are the tasks in your list: "); | ||
for (int i = 0; i < counter; i++) { | ||
System.out.println( | ||
i + 1 + "." + " " + list_of_tasks[i].getStatusIcon() + " " + list_of_tasks[i].description); | ||
} |
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.
It may be clearer to extract these parts as separate functions to avoid nesting loops.
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 good effort in implementation.More work on SLAP with shorter methods, and classes will be helpful in future iP levels and tP.
src/main/java/Duke.java
Outdated
switch (command[0]) { | ||
case "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.
Switch-case should not have different indentations.
src/main/java/Duke.java
Outdated
} | ||
} | ||
|
||
public static void main(String[] args) { |
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.
Try keeping your main method short and have a parser to deal with interpreting user commands instead
src/main/java/Duke.java
Outdated
String d_by = d[1]; | ||
Deadline(d_description, d_by); | ||
break; | ||
case "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.
Perhaps consider using constants instead of strings, for example, COMMAND_EVENT instead of "event"
src/main/java/Duke.java
Outdated
List(); | ||
break; | ||
case "mark": | ||
Integer m_index = Integer.valueOf(command[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.
m_index, u_index etc. might be easy for the original developer to understand but not for someone else reading it or another person taking over. Consider using clearer naming in this case.
src/main/java/Duke.java
Outdated
public class Duke { | ||
public static void main(String[] args) { | ||
|
||
private static Tasks[] list_of_tasks = new Tasks[101]; |
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.
Consider other data structures, apart from a fixed size array, to make your product more scalable, especially since we are already approaching recess week :)
src/main/java/Duke.java
Outdated
print_action(); | ||
} | ||
|
||
public static void Event(String description, String start, String end) { |
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.
Avoid using constructor names of other classes as method name. It can be confusing and potential source of bug. Also, consider separate classes for adding new tasks.
Added JavaDoc comments
No description provided.