[Bingyuan] iP#230
Conversation
theopin
left a comment
theopin
left a comment
There was a problem hiding this comment.
Good job on implementation of duke so far! Some general pointers to improve your code:
- magic literals. Avoid passing in string/int literals into functions directly as it can be difficult for
readers to understand your code. Do consider refactoring them into named constants - Main method seems to be too long. Perhaps consider trying to break it into segments and then refactoring them into seperate methods in order to better visualize its overall flow.
| public String getStatusIcon() { | ||
| return (isDone ? "X" : " "); // mark done task with X | ||
| } |
There was a problem hiding this comment.
Do consider replacing such string literals with named constants, it will help in enhancing readability of your code
| if (command.equals("mark")) { | ||
| Duke.changeTaskState(true, Integer.parseInt(phrase)); | ||
| } else if (command.equals("unmark")) { | ||
| Duke.changeTaskState(false, Integer.parseInt(phrase)); | ||
| } else if (command.equals("event")) { | ||
| Duke.addEvent(phrase); | ||
| } else if (command.equals("todo")) { | ||
| Duke.addTodo(phrase); | ||
| } else if (command.equals("deadline")) { | ||
| Duke.addDeadline(phrase); | ||
| } else if (command.equals("delete")) { | ||
| Duke.delete(Integer.parseInt(phrase)); | ||
| } else { | ||
| System.out.println("Invalid command"); | ||
| } |
There was a problem hiding this comment.
Main seems to be overloaded with a lot of functionality, perhaps consider abstracting behaviour such as parsing command (like this block) into another method?
| String command = inputArgs[0]; | ||
| boolean taskStatus = Boolean.parseBoolean(inputArgs[1]); | ||
| switch (command) { | ||
| case "T": |
There was a problem hiding this comment.
case line should not be tabbed (ie. on the same level as switch)
|
|
||
| private void addFileData(String[] inputArgs) { | ||
| Task newTask; | ||
| String command = inputArgs[0]; |
There was a problem hiding this comment.
Do consider renaming inputArgs to its full form for better readability (ie. inputArguments)
| try { | ||
| if (!file.createNewFile()) { | ||
| Scanner fileData = new Scanner(file); | ||
| while (fileData.hasNext()) { | ||
| data = fileData.nextLine(); | ||
| String[] inputArgs = data.split("|"); | ||
| addFileData(inputArgs); | ||
| } | ||
| fileData.close(); | ||
| } | ||
| } catch (IOException e) { | ||
| System.out.print("\nError getting file data"); | ||
| } | ||
| System.out.println("These are the tasks from your file:\n"); | ||
| list(); | ||
| } |
There was a problem hiding this comment.
try-catch should have a finally clause defined to mark the relevant section of code (in this case 117-118) that runs regardless of the outcome of try catch
2 participants
Week 4 PR