[Ryan Chew] Duke Increments by iltep64 · Pull Request #355 · nus-cs2103-AY1920S1/duke

iltep64 · 2019-09-02T15:39:26Z

(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.)

Add toolVersion block in to Gradle code sample to prevent errors.

Refactor out command handlers into functions, dispatched by map.

…t instance

…e block.

…ist<>

This is to allow alternative UI implementations.

garylyp

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.

garylyp · 2019-09-03T16:19:06Z

src/main/java/org/duke/Duke.java

+            throw new DukeException("I'm sorry, but I don't know what that means. :-(");
+        });
+        this.io.setCommandDispatcher(this.dispatcher);
+


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.

garylyp · 2019-09-03T16:25:01Z

src/main/java/org/duke/cmd/Command.java

+    public final String type;
+    public final String arguments;
+    public final Map<String, String> namedArguments;
+    public Command(String type, String arguments, Map<String, String> namedArguments) {


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.

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.

kelvinnharris · 2019-09-04T12:42:54Z

src/main/java/CounterDecorator.java

@@ -0,0 +1,13 @@
+import java.util.function.Function;
+
+class CounterDecorator implements Function<String, String> {


I like that you create a class to format the string representation of a Task.

kelvinnharris

An eye-opening and inspiring piece with many additional features! I kinda skimmed through to understand what's going on and there's definitely a lot to learn for me.

kelvinnharris · 2019-09-05T05:32:30Z

src/main/java/Command.java

+		if(splits.length > 1) {
+			//Each split value is "switch-name args..."
+			//with no preceding slash
+			for(int i=1;i<splits.length;i++) {


I think it would be more readable to include spaces to become for (int i = 1; i < splits.length; i++)

kelvinnharris · 2019-09-05T05:37:14Z

src/main/java/Command.java

+				String switchName = switchArgs[0];
+				//If no arguments, leave as empty string
+				String switchArguments = switchArgs.length > 1 ? switchArgs[1] : "";
+				namedArguments.put(switchName, switchArguments);


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?

kelvinnharris · 2019-09-05T05:48:16Z

src/main/java/org/duke/Duke.java

+                "  " + t,
+                String.format("Now you have %d task%s in the list.",
+                        this.taskList.size(),
+                        this.taskList.size() == 1 ? "" : "s")


Neat formatting!

kelvinnharris · 2019-09-05T05:56:42Z

src/main/java/org/duke/Duke.java

+        return true;
+    }
+
+    private boolean displayList(Command input) {


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 :')

nishcafe · 2019-09-12T03:49:17Z

src/main/java/org/duke/Duke.java

+	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()


Nice use of streams here!

nishcafe · 2019-09-12T03:52:15Z

src/main/java/org/duke/json/JsonParser.java

+                    throw new JsonException("Missing comma between fields");
+                }
+                String fieldName = this.readString();
+                this.eatWhitespace();


Good use of the SLAP principle here, everything is really easy to read

nishcafe · 2019-09-12T03:52:50Z

src/main/java/org/duke/json/JsonWriter.java

+    }
+
+    private abstract class BlockContext implements AutoCloseable {
+        private static final String separator = ",";


Good use of access modifiers

j-lum and others added 30 commits August 6, 2019 15:25

Add support for Gradle workflow

65f72a8

Add sample checkstyle configuration

0112efe

Add toolVersion block in to Gradle code sample to prevent errors.

Added Greet, Echo, Exit features

7faa65a

Implement list add and show features.

46c55c6

Created Task class to represent tasks.

94b4414

Refactor out command handlers into functions, dispatched by map.

Implemented marking a task as done.

4cec14a

Refactor IO interface into DukeIO instance, and Duke logic into objec…

013cc37

…t instance

Implement deadline task class and addition logic

9303e1d

Refactor out and improve task-adding logic.

5abe5d0

Create a uniform command interface, with parsing logic.

11f4134

Implement dialog block logic, so messages get aggregated into a singl…

3ec4d4e

…e block.

Add header to list command output.

b79c5c0

Add Event task and make it available by the 'event' command.

3a0d632

Add basic error handling support.

0139127

Added deletion functionality.

16718fb

Implement basic JSON decoder

8dd4b9b

Add basic JSON writer

d7e9187

Add type registry for JSON serializiation

501ae91

Add convenience functions for JSON encoding/decoding.

9880921

Refactor task types to enums

db72071

Add JSON builder class for Tasks

6ca4441

Add listOf function, and produce ArrayList<> values instead of just L…

1cf63ce

…ist<>

Use JSON to load/store task list

f5ccf6e

Fix completed status not being loaded

a0d17c1

Add date parsing logic

0b162a2

Have EventTask and DeadlineTask to use date logic.

c842c6f

Merge branch 'branch-Level-7'

355f065

Merge branch 'branch-Level-8'

93b6e1c

Add encoder lookup cache for Registry, and avoid printing debug messages

dce444a

Move all classes into (sub)packages of org.duke

45172d8

iltep64 added 12 commits September 2, 2019 23:51

Add task find command

9f28efa

Reformat code to satisfy standards

ed5ea49

Add javadocs to public classes and methods.

8c16531

Merge branch 'branch-Level-9'

c907b2c

Merge branch 'codingstandard'

c5eee9a

Merge branch 'javadocs'

af1b0d4

Merge branch 'gradle'

7e2227e

Add JAR build plugin.

e0b6066

Add tests for Command and DateParser, and automate via Gradle.

63d9c81

Refactor out IO and command parsing.

3bfe760

This is to allow alternative UI implementations.

Implement a basic JavaFX UI.

1e6e594

Merge branch 'branch-Level-10'

31fb5b4

garylyp reviewed Sep 3, 2019

View reviewed changes

kelvinnharris reviewed Sep 4, 2019

View reviewed changes

kelvinnharris reviewed Sep 5, 2019

View reviewed changes

nus-se-pr-bot added team.4 tutorial.CS2103T-W11 tutor.kylase-learning labels Sep 6, 2019

nishcafe reviewed Sep 12, 2019

View reviewed changes

iltep64 added 8 commits September 18, 2019 00:53

Encapsulate Command fields.

b04abda

Beautify DukeFx UI, by adding colored message boxes for Duke and user.

354adca

Extract command handlers into their own classes, and add a help command.

f653e2d

Refactor storage out into own class, and improve CounterDecorator.

7ea74be

Refactored JavaFX UI to use FXML custom components.

00772ef

Fix JavaFX startup classpath issues.

6d9eb22

Add user guide/documentation.

1eebc7b

Set theme jekyll-theme-slate

b508421

		@@ -0,0 +1,13 @@
		import java.util.function.Function;

		class CounterDecorator implements Function<String, String> {

Conversation

iltep64 commented Sep 2, 2019

Uh oh!

garylyp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kelvinnharris left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants