[Lee Zhi Zhong Moses] iP #227
Conversation
…ed IndexOutOfBounds error
There was a problem hiding this comment.
Well done in catching up in the past one week! Keep the spirit up! So far most of your code follows well in terms of coding standards and coding quality. You might want to consider extracting methods out more often whenever appropriate to prevent one single method to be too long and to avoid issues such as arrow code. Below are some suggestions on the possible things you can look out for. All the best for further levels!
src/main/java/Duke.java
Outdated
| String exitCommand = "bye"; | ||
| String toDo = "list"; | ||
| String checkAsDone = "mark"; |
There was a problem hiding this comment.
You might want to consider converting them into named constants instead of a variables since it is unlikely for them to be changed.
E.g. public static final String EXIT_COMMAND = "bye";
src/main/java/Duke.java
Outdated
| private static Scanner in = new Scanner(System.in); | ||
| private static final Task[] tasks = new Task[100]; | ||
|
|
||
| public static void command(String line) { |
There was a problem hiding this comment.
A better naming practices for methods is to start it with a verb. For example, you can call this executeCommand or something better here. (While the word command can be a verb too, it doesn't suit your usage very well)
| String[] tokens = line.split(" ", 2); | ||
| String command = tokens[0]; | ||
|
|
||
| while (!command.equals(exitCommand)) { |
There was a problem hiding this comment.
Currently, this entire method is too long. You can try to keep a method short by extracting its content out to other methods whenever appropriate.
|
|
||
| for (int i = 0; i < numberOfTasks; ++i) { | ||
| System.out.format("%d. ", (i + 1)); | ||
| System.out.println(tasks[i]); |
There was a problem hiding this comment.
This is a good use of polymorphism here. You can apply a similar concept when dealing with similar scenarios in further levels.
|
|
||
| @Override | ||
| public String toString() { | ||
| return "[T]" + super.toString(); |
There was a problem hiding this comment.
Arguably, strings like [T] can be seen as a magic strings. You might want to consider extracting strings like this into a named constants. Sometimes, you could also use enums to achieve similar effect. There could be other magic strings/numbers/literals in your other code too that you might want to look out for them.
src/main/java/Duke.java
Outdated
| String task = tokens[1]; | ||
| switch(command) { | ||
| case "todo": | ||
| Task t = new ToDo(task); |
There was a problem hiding this comment.
We want to avoid using a single character as the name of a variable (with some exceptions such as i, j, k in a for loop). Try to think if another name for this
| public class Duke { | ||
|
|
||
| private static Scanner in = new Scanner(System.in); | ||
| private static final Task[] tasks = new Task[100]; |
There was a problem hiding this comment.
For program scalability, you might want to consider using ArrayList instead of setting a maximum number of tasks user can have here.
| } | ||
|
|
||
| public String getStatus() { | ||
| return (isDone ? "[X]" : "[ ]"); // mark done task with X |
There was a problem hiding this comment.
You might want to capitalise the first character in your comment.
No description provided.