Conversation
astralum
left a comment
astralum
left a comment
There was a problem hiding this comment.
Good adherence to coding standards!
| } | ||
| } | ||
| System.out.println(border); | ||
|
|
huien77
left a comment
huien77
left a comment
There was a problem hiding this comment.
I can see some deep nesting and I think it would be good to create more methods to solve that
I also find some magic numbers, it would be good to avoid them
Overall, very good
| + "|_____| | ||_____| \n"; | ||
| private static String border = "____________________________________________________________\n"; | ||
|
|
||
| public static void addedTaskMessage(Task task) { |
| String line; | ||
| printStartMessage(); | ||
|
|
||
| do { |
There was a problem hiding this comment.
maybe can move the while loop to a new method, and call that method instead, following single level of abstraction
| int j = 1; | ||
| System.out.println(border); | ||
| System.out.println("Here is your list"); | ||
|
|
||
| for (Task item : items) { | ||
| if (item != null) { | ||
| System.out.print(j + "."); | ||
| System.out.println(item); | ||
| j++; | ||
| } | ||
| } |
There was a problem hiding this comment.
can consider moving this to a new method, might look neater, and no need so many level of indentation
| } | ||
| System.out.println(border); | ||
|
|
||
| } else if (line.length() > 4 && line.substring(0,4).contains("done")) { |
There was a problem hiding this comment.
avoid magic number like 4, instead store them as constants
| private static final int FOUR = 4; | ||
| private static final int FIVE = 5; | ||
| private static final int SIX = 6;; | ||
| private static final int EIGHT = 8; | ||
| private static final int NINE = 9; |
There was a problem hiding this comment.
These string constants to refer to an integer may seem redundant and self-explanatory.
If you are working to replace them with magic numbers I would suggest more meaningful names.
| System.out.println("Type bye to exit\n" + border); | ||
| } | ||
| public static boolean isInvalid(CommandList task, String line , String key) { | ||
| if (!line.split(key)[1].trim().isEmpty()) { |
There was a problem hiding this comment.
This expression is a little complicated - calling 3 methods and a negation. See how to improve the quality of expressions here.
| try { | ||
| if (saveFile.createNewFile()) { | ||
| System.out.println("Successfully created save file"); | ||
| } else { | ||
| System.out.println("Save file already exists"); | ||
| } | ||
| } catch (IOException e) { | ||
| e.printStackTrace(); |
There was a problem hiding this comment.
How about including the previous statements in the try catch block? After all, this method is doing file related operations and the program could catch file related exceptions.
| fw.write(saveList.toString().charAt(1) + "/"); | ||
| fw.write(saveList.getStatus() + "/" + saveList.getDescription()); |
There was a problem hiding this comment.
For the parameters to the file writer, you could have declared variables for each for them to improve readability. E.g. fileStatus = saveList.getStatus().
| System.out.println("Save file successfully loaded"); | ||
| } | ||
|
|
||
| public static String readCommand(Scanner in, CommandList task, DukeException error) { |
There was a problem hiding this comment.
This method is parsing the command string, but it is not obvious how it is done with seemingly arbitrary constants like FOUR FIVE EIGHT etc. You could consider a more direct way of comparing strings and checking parameters.
| public void executeCommand(ArrayList<Task> items, DukeException error, String input) { | ||
| String[] inputs; | ||
| switch (command) { | ||
| case CMD_TODO: |
There was a problem hiding this comment.
This deviates from the switch case statement layout conventions - indentation for case is not needed.
| case CMD_EVENT: | ||
| inputs = input.split(AT); | ||
| addEvent(items, inputs[0].trim().replace(EVENT, "").trim(), inputs[1].trim()); | ||
| addTaskMessage(items.get(taskCount)); | ||
| break; | ||
| case CMD_DEADLINE: | ||
| inputs = input.split(BY); | ||
| addDeadline(items, inputs[0].trim().replace(DEADLINE, "").trim(), inputs[1].trim()); | ||
| addTaskMessage(items.get(taskCount)); | ||
| break; | ||
| case CMD_LIST: | ||
| int j = 1; | ||
| System.out.println(border); | ||
| System.out.println("Here are the task in your list:"); | ||
| for (Task item : items) { | ||
| if (item != null) { | ||
| System.out.print(j + "."); | ||
| System.out.println(item); | ||
| j++; | ||
| } | ||
| } | ||
| break; | ||
| case CMD_DONE: | ||
| int dividerPosition = input.indexOf(" ") + 1; | ||
| int endPosition = input.length(); | ||
| if (endPosition > FIVE) { | ||
| String num = input.substring(dividerPosition, endPosition); | ||
| int taskNum = Integer.parseInt(num) - 1; | ||
| items.get(taskNum).markDone(); | ||
| System.out.println(border + "Nice! task is done " + '\n' + border); | ||
| } | ||
| break; | ||
| case CMD_TERMINATE: | ||
| printEndMessage(); | ||
| break; | ||
| case CMD_DELETE: | ||
| dividerPosition = input.indexOf(" ") + 1; | ||
| endPosition = input.length(); | ||
| if (endPosition > SEVEN) { | ||
| String num = input.substring(dividerPosition, endPosition); | ||
| int taskNum = Integer.parseInt(num) - 1; | ||
| removeItem(items, taskNum); | ||
| } | ||
| break; | ||
| default: | ||
| printErrorMessage(error); | ||
| break; | ||
| } |
There was a problem hiding this comment.
While you have organized different procedure into separate cases, the executeCommand method is still rather long. How about trying SLAP principle here?
| } | ||
| } | ||
|
|
||
| private static void readSave(CommandList task) throws FileNotFoundException { |
There was a problem hiding this comment.
It would be good to provide method descriptions as comments/javadocs to improve code readability.
Added Date and Time functionality
Added Find Feature
Documentation for A-JavaDoc
4 participants
No description provided.