[javienneyeo] ip #215
[javienneyeo] ip #215
Conversation
There was a problem hiding this comment.
Good job in splitting the classes with similar nature into the same packages. The classes created by you now are also on point too. So far, I don't see any issue w.r.t. coding standards. Keep the good work going! Below are some comments on some minor coding quality issues and some suggestions.
| @@ -0,0 +1,6 @@ | |||
| package duke.exceptions; | |||
|
|
|||
| public class UnknownCommandException extends Exception { | |||
There was a problem hiding this comment.
Instead of having the content of the exception empty, you can consider doing some interesting and useful stuff within the exception, such as declaring a function to print the error message.
src/main/java/Duke.java
Outdated
| private static void createEvent(Task[] tasks, String task) { | ||
| String[] words = task.split("/from"); | ||
| String description = words[0]; | ||
| String[] words2 = words[1].split("/to"); |
There was a problem hiding this comment.
Instead of calling it words and words2, you might want to think of other more related or intuitive namings for them
src/main/java/Duke.java
Outdated
|
|
||
| private static void editList() throws UnknownCommandException, EmptyDescriptionException, TaskToMarkDoesNotExistException { | ||
| String[] splitText = getInput(); | ||
| Task[] tasks = new Task[100]; |
There was a problem hiding this comment.
Your current code structure only accepts a maximum of 100 tasks for now. Moving on, you might want to make this more flexible by using ArrayList.
src/main/java/Duke.java
Outdated
| default: | ||
| throw new UnknownCommandException(); | ||
| } |
| if (typeOfTask.equals("marked") || typeOfTask.equals("unmarked")) { | ||
| System.out.println("OOPS!! Task to be " + typeOfTask + " was not specified!"); | ||
| } else { | ||
| System.out.println("OOPS!! The description of " + typeOfTask + " cannot be empty!"); | ||
| } |
There was a problem hiding this comment.
Instead of checking whether it is marked or unmarked and proceed to print a different error message for it, you might want to just create another exception separately for the mark and unmark feature.
src/main/java/Duke.java
Outdated
|
|
||
| private static String[] getInput() { | ||
| Scanner input = new Scanner(System.in); | ||
| String text = input.nextLine(); // input the whole sentence into text |
There was a problem hiding this comment.
Generally, we would want to capitalise the first letter in our comment.
src/main/java/Duke.java
Outdated
| System.out.println("Here are the tasks in your list:"); | ||
| for (int i = 0; i < Task.numberOfTasks; i += 1) { | ||
| System.out.print(i + 1 + ". "); | ||
| tasks[i].printTask(); |
There was a problem hiding this comment.
This is a good usage of polymorphism. When you are implementing the features in future levels, you might want to think about using similar techniques for other appropriate features too.
src/main/java/Duke.java
Outdated
| @@ -1,10 +1,138 @@ | |||
| import duke.tasks.Task; | |||
There was a problem hiding this comment.
I noticed that your Duke.java file is outside the duke folder. You might want to move it into that folder such that all your code is under the folder. You could use the move class feature under refactor to achieve it.
|
|
||
| @Override | ||
| public void printTask() { | ||
| System.out.println("[D][" + getStatusIcon() + "] " + description + "(by:" + end + ")"); |
There was a problem hiding this comment.
Arguably, strings like [D] 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.
added find task functionality
No description provided.