Conversation
major refactor, add error handling
TVageesan
left a comment
TVageesan
left a comment
There was a problem hiding this comment.
Overall really well done. It's clear you've had some experience coding before (or you actually watch the lectures) as your code is clean, readable and easy to reason about. I've made some nitpicks about readability as well as a additional (optional) note you can consider to even further improve it.
| switch (command) { | ||
| case "list": | ||
| case "l": | ||
| return new ListCommand(taskManager, ui); | ||
| case "add": | ||
| case "a": | ||
| return parseAddCommand(arguments); | ||
| case "mark": | ||
| case "m": | ||
| return parseMarkCommand(arguments, true); | ||
| case "unmark": | ||
| case "u": | ||
| return parseMarkCommand(arguments, false); | ||
| case "delete": | ||
| case "d": | ||
| return parseDeleteCommand(arguments); | ||
| //case "undo": | ||
| //case "x": | ||
| // implement undo last functionality | ||
| case "help": | ||
| case "h": | ||
| return new HelpCommand(ui); | ||
| case "exit": | ||
| case "e": | ||
| //bb.greetGoodbye(); | ||
| return new ExitCommand(); | ||
| default: | ||
| System.out.println("Unknown command."); | ||
| return new HelpCommand(ui); | ||
| } | ||
| } |
There was a problem hiding this comment.
Like the initiative to do short form commands for each command. However, these are considered magic strings. Perhaps you could put them in a constant EXIT_COMMAND/EXIT_COMMAND_SHORT or use a enum to represent the commands.
| // System.out.println(randomEnum(TaskType.class).name()); | ||
| // return new Command(); | ||
| } | ||
|
|
||
| /** | ||
| * Taken from stackoverflow.com/questions/1972392/pick-a-random-value-from-an-enum | ||
| * from users Eldelshell and zuddduz. | ||
| * @return | ||
| */ | ||
| public static <T extends Enum<?>> T randomEnum(Class<T> clazz) { | ||
| int x = random.nextInt(clazz.getEnumConstants().length); | ||
| return clazz.getEnumConstants()[x]; | ||
| } |
There was a problem hiding this comment.
As it is, this function does not seem to serve any functionality. Consider removing it in your final version of IP.
| enum TaskType { | ||
| DEADLINE, D, | ||
| EVENT, E, | ||
| TODO, T | ||
| } |
There was a problem hiding this comment.
Consider putting this in its own file. Enums, like classes, should have a dedicated file. You could then even share it across multiple classes for consistency.
src/main/java/bluebird/Main.java
Outdated
| } catch (IndexOutOfBoundsException e) { | ||
| ui.showError(e.getMessage()); | ||
| } | ||
| // String userInput = getUserInput(); |
There was a problem hiding this comment.
If the following section of code is no longer used, consider deleting it. Trust your version control tools to keep track of old versions of your code you want to reference instead of keeping dead code around.
| } | ||
|
|
||
| private static Event createEvent(String details) throws IllegalTaskParameterException { | ||
| String[] parts = details.split("/from|/to|/f|/t", 3); |
There was a problem hiding this comment.
The .split pattern here can be put into a constant i.e. EVENT_ARG_PATTERN for better readability. This principle applies for the other create functions as well.
| public List<Task> getTasks() { | ||
| return Collections.unmodifiableList(tasks); | ||
| } |
There was a problem hiding this comment.
Good idea to generate a copy of your list instead of directly returning a reference to your list! It's clear you understand the importance of protecting our private data within classes.
(This part is optional, does not affect code quality if you leave it as is)
You could even further improve on this by defining specific access methods for specific use cases instead of a generic getTasks(). Looking at your code, you currently use getTasks() to 1) print out the current tasks you have in your list 2) get the size of the list. Could we define specific methods getSize() to return the number of tasks and toString() to format the list into a printable string? This way our raw "tasks" data never gets passed out from the class.
| setBy(by); | ||
| } | ||
|
|
||
| public String getBy() { | ||
| return by; | ||
| } | ||
|
|
||
| public void setBy(String by) { | ||
| this.by = by; | ||
| } |
There was a problem hiding this comment.
do setBy() and getBy() serve a purpose in your overall program? In this scenario, simply this.by = by; will actually be more succinct. getters and setters should only be implemented as and when necessary. This will help reduce class length. You can apply this to your other tasks files as well.
add undo last functionality
Changed length of line separator
2 participants
No description provided.