[Feng Guangyao] iP#69
Conversation
| switch (firstCommand) { | ||
| case "bye": { | ||
| running = false; | ||
| break; | ||
| } | ||
| case "todo": { | ||
| Task newTask = new Todo(input.substring(5, input.length())); | ||
| insertTask(newTask); | ||
| break; | ||
| } | ||
| case "deadline": { | ||
| int[] dividerPositions = new int[] { input.indexOf("/") }; | ||
| Task newTask = new Deadline(input.substring(9, dividerPositions[0] - 1), | ||
| input.substring(dividerPositions[0] + 4, input.length())); | ||
| insertTask(newTask); | ||
| break; | ||
| } | ||
| case "event": { | ||
| int[] dividerPositions = new int[] { input.indexOf("/"), 0 }; | ||
| // Try to find the second "/" | ||
| String remainingString = input.substring(dividerPositions[0] + 1, input.length()); | ||
| dividerPositions[1] = dividerPositions[0] + 1 + remainingString.indexOf("/"); | ||
| Task newTask = new Event(input.substring(6, dividerPositions[0] - 1), | ||
| input.substring(dividerPositions[0] + 6, dividerPositions[1] - 1), | ||
| input.substring(dividerPositions[1] + 3, input.length())); | ||
| insertTask(newTask); | ||
| break; | ||
| } | ||
| case "list": { | ||
| showTasks(); | ||
| break; | ||
| } | ||
| default: { | ||
| showLog(input); | ||
| } | ||
| } |
There was a problem hiding this comment.
Switch case statement does not follow the coding standard
| int[] dividerPositions = new int[] { input.indexOf("/"), 0 }; | ||
| // Try to find the second "/" | ||
| String remainingString = input.substring(dividerPositions[0] + 1, input.length()); | ||
| dividerPositions[1] = dividerPositions[0] + 1 + remainingString.indexOf("/"); | ||
| Task newTask = new Event(input.substring(6, dividerPositions[0] - 1), | ||
| input.substring(dividerPositions[0] + 6, dividerPositions[1] - 1), | ||
| input.substring(dividerPositions[1] + 3, input.length())); |
There was a problem hiding this comment.
These lines can be quite confusing and not very readable. Perhaps renaming the dividerPositions to another name would be better. Similar cases throughout.
| public class Main { | ||
| public static void main(String[] args) { | ||
| // create a todo task and print it | ||
| Todo t = new Todo("Read a good book"); | ||
| System.out.println(t); | ||
| // change todo fields and print again | ||
| t.setDone(true); | ||
| System.out.println(t); | ||
| // create a deadline task and print it | ||
| Deadline d = new Deadline("Read textbook", "Nov 16"); | ||
| System.out.println(d); | ||
| // change deadline details and print again | ||
| d.setDone(true); | ||
| d.setBy("Postponed to Nov 18th"); | ||
| System.out.println(d); | ||
| } |
There was a problem hiding this comment.
Not sure what is the main reason for this class as a main method is already written in Duke.java. Would it be for testing? Perhaps using the appropriate platform for testing would be better.
| public Deadline(String description, String by) { | ||
| super(description); | ||
| this.by = by; | ||
| isDone = false; | ||
| type = "D"; | ||
| } |
There was a problem hiding this comment.
Perheps you can change the parameter by to some noun phrase to be more clear. The type = 'D' looks a bit confusing, especially if the reviwer don't know we need to print out the type of task as 'D'.
There was a problem hiding this comment.
actually you could rename it as taskType
| showLog("Bye. Hope to see you again soon!"); | ||
| } | ||
|
|
||
| public static void showLog(Object content) { |
There was a problem hiding this comment.
May I know the meaning of showLog? Maybe you can extend it to full name if it is just some short phrase
|
|
||
| public class Duke { | ||
| static Task[] tasks = new Task[1000]; | ||
| static int amount = 0; |
There was a problem hiding this comment.
The variable amount keeps track of the number of tasks, right? I would suggest using taskCount instead.
| switch (firstCommand) { | ||
| case "bye": { | ||
| running = false; | ||
| break; | ||
| } | ||
| case "todo": { | ||
| Task newTask = new Todo(input.substring(5, input.length())); | ||
| insertTask(newTask); | ||
| break; | ||
| } | ||
| case "deadline": { | ||
| int[] dividerPositions = new int[] { input.indexOf("/") }; | ||
| Task newTask = new Deadline(input.substring(9, dividerPositions[0] - 1), | ||
| input.substring(dividerPositions[0] + 4, input.length())); | ||
| insertTask(newTask); | ||
| break; | ||
| } | ||
| case "event": { | ||
| int[] dividerPositions = new int[] { input.indexOf("/"), 0 }; | ||
| // Try to find the second "/" | ||
| String remainingString = input.substring(dividerPositions[0] + 1, input.length()); | ||
| dividerPositions[1] = dividerPositions[0] + 1 + remainingString.indexOf("/"); | ||
| Task newTask = new Event(input.substring(6, dividerPositions[0] - 1), | ||
| input.substring(dividerPositions[0] + 6, dividerPositions[1] - 1), | ||
| input.substring(dividerPositions[1] + 3, input.length())); | ||
| insertTask(newTask); | ||
| break; | ||
| } | ||
| case "list": { | ||
| showTasks(); | ||
| break; | ||
| } | ||
| default: { | ||
| showLog(input); | ||
| } | ||
| } |
There was a problem hiding this comment.
I like the way that you try to group the whole things as OO style.
| type = "T"; | ||
| } | ||
|
|
||
| public boolean isDone() { |
There was a problem hiding this comment.
isDone is a verb phrase. I guess you want to return get the isDone field. It would be better if you could use "getisDone" or "getFinishStatus", sth. like these.
Branch-A-JavaDoc
3 participants
just for test