[Ryan Chew] Duke Increments #355
Conversation
Add toolVersion block in to Gradle code sample to prevent errors.
Refactor out command handlers into functions, dispatched by map.
This is to allow alternative UI implementations.
There was a problem hiding this comment.
Hi Ryan, I decided to review yours since one of my assignees hasn't submitted his pull request yet.
Pretty impressive work, with plenty of additional features being implemented to improve the flow of the application and cover all corner cases. The json package was extremely substantial, though I am not too sure if the amount of data stored would actually warrant such intricate designs.
Nevertheless, it was refreshing and eye-opening to see varied applications of new Java features and functional paradigms put in play.
| throw new DukeException("I'm sorry, but I don't know what that means. :-("); | ||
| }); | ||
| this.io.setCommandDispatcher(this.dispatcher); | ||
|
|
There was a problem hiding this comment.
I like how you implemented all your commands with a central dispatcher. It is very neat and shows clearly what text is bound to each command. Also like how you had a default fallback command handler to handle unknown commands from user input.
| public final String type; | ||
| public final String arguments; | ||
| public final Map<String, String> namedArguments; | ||
| public Command(String type, String arguments, Map<String, String> namedArguments) { |
There was a problem hiding this comment.
I felt that the access modifiers for the above fields could be set to private to prevent its values from being modified freely by other classes.
There was a problem hiding this comment.
The Command class is intended to be an immutable value type, akin to a C struct. The fields cannot be modified, as they are final (bar the Map which has interior mutability). You still have a point in that it breaks encapsulation, so future updates may break consumers of the API.
At the very least, I'll fix it up for the Map case by wrapping it with Collections.unmodifiableMap(). Thanks for the catch.
src/main/java/CounterDecorator.java
Outdated
| @@ -0,0 +1,13 @@ | |||
| import java.util.function.Function; | |||
|
|
|||
| class CounterDecorator implements Function<String, String> { | |||
There was a problem hiding this comment.
I like that you create a class to format the string representation of a Task.
src/main/java/Command.java
Outdated
| if(splits.length > 1) { | ||
| //Each split value is "switch-name args..." | ||
| //with no preceding slash | ||
| for(int i=1;i<splits.length;i++) { |
There was a problem hiding this comment.
I think it would be more readable to include spaces to become for (int i = 1; i < splits.length; i++)
src/main/java/Command.java
Outdated
| String switchName = switchArgs[0]; | ||
| //If no arguments, leave as empty string | ||
| String switchArguments = switchArgs.length > 1 ? switchArgs[1] : ""; | ||
| namedArguments.put(switchName, switchArguments); |
There was a problem hiding this comment.
I find it a bit confusing to use HashMap and pass them around as there would be a need to remember the keys. May I know why you decided to proceed with it?
src/main/java/org/duke/Duke.java
Outdated
| " " + t, | ||
| String.format("Now you have %d task%s in the list.", | ||
| this.taskList.size(), | ||
| this.taskList.size() == 1 ? "" : "s") |
src/main/java/org/duke/Duke.java
Outdated
| return true; | ||
| } | ||
|
|
||
| private boolean displayList(Command input) { |
There was a problem hiding this comment.
Is there any other purpose of returning a boolean instead of void besides passing it as a predicate to the dispatcher? Sorry, I'm a bit lost :')
src/main/java/org/duke/Duke.java
Outdated
| private boolean findTasks(Command input) { | ||
| this.io.say("Here are the matching tasks in your list:"); | ||
| String target = input.arguments.toLowerCase(); | ||
| this.io.say(this.taskList.stream() |
| throw new JsonException("Missing comma between fields"); | ||
| } | ||
| String fieldName = this.readString(); | ||
| this.eatWhitespace(); |
There was a problem hiding this comment.
Good use of the SLAP principle here, everything is really easy to read
| } | ||
|
|
||
| private abstract class BlockContext implements AutoCloseable { | ||
| private static final String separator = ","; |
(Note: I couldn't get Github to initiate a pull request from an older commit in master, so I have to use a new branch.)