[Yifan] ip #70
[Yifan] ip #70
Conversation
src/main/java/Duke.java
Outdated
| /** | ||
| * | ||
| */ | ||
| private static final int MAXLISTNUM = 100; |
There was a problem hiding this comment.
Constant names requires underscore to separate words.
src/main/java/Duke.java
Outdated
| final String commandType = commandTypeAndParams[0]; | ||
| final String commandArgs = commandTypeAndParams[1]; | ||
| switch (commandType) { | ||
| case "todo": |
There was a problem hiding this comment.
Consider using a constant string instead of magic string for the cases
src/main/java/Event.java
Outdated
| return response; | ||
| } | ||
|
|
||
| protected String[] parseDescriptions(String commandArgs) { |
There was a problem hiding this comment.
Good that you followed the naming convention
src/main/java/Todo.java
Outdated
| @@ -0,0 +1,11 @@ | |||
| public class Todo extends Task{ | |||
There was a problem hiding this comment.
There seems to be a missing space before {
merge the branch 7
| while (true) { | ||
| String inputText = scanner.nextLine(); | ||
| while (inputText.trim().isEmpty()) | ||
| inputText = scanner.nextLine(); | ||
| try { | ||
| isProgramEnd = taskManager.handleTask(inputText); | ||
| } catch (DukeException e) { | ||
| e.handleError(); | ||
| } | ||
| if (isProgramEnd) { | ||
| System.exit(0); | ||
| } | ||
| } |
There was a problem hiding this comment.
[Code Quality] The happy path in this section could be more fleshed out.
src/main/java/DukeException.java
Outdated
| case "TaskTypeError": | ||
| errorMessage = " OOPS!!! I'm sorry, but I don't know what that means :-("; | ||
| break; | ||
| case "TodoDescriptionError": | ||
| errorMessage = " OOPS!!! The description of a todo cannot be empty."; | ||
| break; | ||
| case "EventDescriptionError": | ||
| errorMessage = " OOPS!!! The description of a event is wrong."; | ||
| break; | ||
| case "DeadlineDescriptionError": | ||
| errorMessage = " OOPS!!! The description of a deadline is wrong."; | ||
| break; | ||
| case "RestorationFileCorrupted": | ||
| errorMessage = " OOPS!!! File Restoration Corrupted. Restoration Process Stopped."; | ||
| break; | ||
| default: | ||
| errorMessage = " OOPS!!! Some unknown errors happen."; |
src/main/java/Record.java
Outdated
| try { | ||
| boolean isFileCreated = file.createNewFile(); | ||
| } catch (IOException e) { | ||
| e.printStackTrace(); | ||
| } | ||
| } else { | ||
| try { | ||
| readFileAndRestore(file); | ||
| } catch (FileNotFoundException e) { | ||
| e.printStackTrace(); | ||
| } |
There was a problem hiding this comment.
[Code Quality] The try-catch clauses can be combined to improve readability and control flow. Also, do keep in mind that you should remove printStackTrace() in production-grade code. Figure out why?
src/main/java/Task.java
Outdated
| protected boolean isDone; | ||
|
|
||
| public Task(String commandArgs) { | ||
| //descriptions = parseDescriptions(commandArgs); |
There was a problem hiding this comment.
[Code Quality] Please remove the unused comment.
src/main/java/TaskManager.java
Outdated
| //private static final int MAX_LIST_NUM = 100; | ||
| //private static Task[] taskList = new Task[MAX_LIST_NUM]; | ||
| private static ArrayList<Task> taskList = new ArrayList<Task>(); | ||
| //private static int listLength = 0; |
There was a problem hiding this comment.
[Code Quality] Again, please remove the unused comments to reduce clutter in the codebase.
src/main/java/TaskManager.java
Outdated
| public class TaskManager { | ||
| //private static final int MAX_LIST_NUM = 100; | ||
| //private static Task[] taskList = new Task[MAX_LIST_NUM]; | ||
| private static ArrayList<Task> taskList = new ArrayList<Task>(); |
There was a problem hiding this comment.
[Code Design] For future iterations, you can consider whether taskList should be static or not.
src/main/java/TaskManager.java
Outdated
| case "todo": | ||
| Todo todo = new Todo(commandArgs); | ||
| taskList.add(todo); | ||
| response = taskList.get(taskList.size()-1).getResponse(); | ||
| Util.printTaskResponse(response, taskList.size()); | ||
| updateRestorationFile(); | ||
| break; | ||
| case "deadline": | ||
| Deadline deadline = new Deadline(commandArgs); | ||
| taskList.add(deadline); | ||
| response = taskList.get(taskList.size()-1).getResponse(); | ||
| Util.printTaskResponse(response, taskList.size()); | ||
| updateRestorationFile(); | ||
| break; | ||
| case "event": | ||
| Event event = new Event(commandArgs); | ||
| taskList.add(event); | ||
| response = taskList.get(taskList.size()-1).getResponse(); | ||
| Util.printTaskResponse(response, taskList.size()); | ||
| updateRestorationFile(); | ||
| break; | ||
| case "list": | ||
| showAllTasks(); | ||
| break; | ||
| case "delete": | ||
| index = Integer.valueOf(commandArgs); | ||
| response = taskList.get(index-1).getResponse(); | ||
| taskList.remove(index-1); | ||
| Util.printDeleteResponse(response, taskList.size()); | ||
| updateRestorationFile(); | ||
| break; | ||
| case "mark": | ||
| index = Integer.valueOf(commandArgs); | ||
| taskList.get(index-1).setStringState(true); | ||
| response = taskList.get(index-1).getResponse(); | ||
| Util.printMarkResponse(response); | ||
| updateRestorationFile(); | ||
| break; | ||
| case "unmark": | ||
| index = Integer.valueOf(commandArgs); | ||
| taskList.get(index-1).setStringState(false); | ||
| response = taskList.get(index-1).getResponse(); | ||
| Util.printUnmarkResponse(response); | ||
| updateRestorationFile(); | ||
| break; | ||
| case "bye": | ||
| Util.showExitMessage(); | ||
| return true; |
There was a problem hiding this comment.
[Code Quality] Consider isolating/extracting the logic in each case for better SLAP. Another benefit of doing this is also to reduce the complexity of this entire method. Remember that functions should be small.
src/main/java/Util.java
Outdated
| @@ -0,0 +1,61 @@ | |||
| public class Util { | |||
There was a problem hiding this comment.
[Code Design] Good that you make a dedicated utility class. But I think the methods here are more suited to a separate UI class.
several new functions are added