react: add to track #29

petertseng · 2017-01-18T09:01:01Z

Tests are taken from https://github.com/exercism/x-common/blob/master/exercises/react/canonical-data.json

There is one extra test to exercise the generics in Kotlin - this will be from the Rust track: https://github.com/exercism/xrust/blob/master/exercises/react/tests/react.rs#L223

Full disclosure: I ported this exercise to a few languages in the past:

The main reason I do this is it's a cheap way for me to get a taste of the language's capabilities, since it touches on a few interesting aspects. In Kotlin: generics, inheritance, visibility, abstract classes, properties and setters, inner classes, anonymous objects that satisfy an interface, higher order functions (declaration and use), constructors (primary and secondary), parameterized tests. So I satisfy my language curiosity, and hopefully, y'all get a usable exercise out of it.

petertseng · 2017-01-18T09:13:46Z

exercises/react/src/example/kotlin/React.kt

+class Reactor<T>() {
+    abstract inner class Cell {
+        abstract val value: T
+        internal val dependents = mutableListOf<ComputeCell>()


interestingly, protected is not possible, because then the c.dependents.add(this) in the constructors all fail compilation with:

:compileKotlin e: /home/pt/xkotlin/exercises/react/src/main/kotlin/React.kt: (22, 15): Cannot access 'dependents': it is protected in 'Cell' e: /home/pt/xkotlin/exercises/react/src/main/kotlin/React.kt: (25, 16): Cannot access 'dependents': it is protected in 'Cell' e: /home/pt/xkotlin/exercises/react/src/main/kotlin/React.kt: (26, 16): Cannot access 'dependents': it is protected in 'Cell' FAILED

This is a little strange to me, since ComputeCell is (I hope?!) a subclass of Cell. Perhaps it doesn't apply inside constructors, or something like that... after all, you could consider the constructor to be a function not associated with any particular class, but it does return an instance of a particular class...?

Yea, I'm likewise surprised that this error shows up for a protected variable when being accessed in a subclass' constructor, it is available in the subclass outside of the constructor though...

petertseng · 2017-01-18T10:11:39Z

All tests from x-common have been implemented. ~~I am planning on adding one more to exercise generics in Kotlin.~~ In case it is not clear, this plan has been executed.

It is expected that by the time the student solution gets to this point, they will already have all they need to pass this test. The only point of this test is to show off the generics, and hopefully show a cool little gadget that can be made from a reactor.

petertseng · 2017-01-18T10:52:59Z

exercises/react/src/example/kotlin/React.kt

+            c2.dependents.add(this)
+        }
+
+        fun addCallback(f: (T) -> Any): Subscription {


Sigh, I changed too many intervening lines, so the old comment is on an "outdated diff" now. Gotta post it again.

I hope this is not too strange of an API. The API looks something like:

val cell: ComputeCell = ... val sub = cell.addCallback { print(it) } // ... sub.cancel()

The alternative is to have addCallback return some sort of ID:

val cell: ComputeCell = ... val subID = cell.addCallback { print(it) } // ... cell.removeCallback(subID)

I really like what you did here returning back a Subscription. Do you think we should provide some initial getting started help by at least providing some interfaces?

Do you think we should provide some initial getting started help by at least providing some interfaces?

I would be in support of that, because it may be otherwise difficult to determine from just the tests that addCallback is supposed to return something that you can call cancel() on. It's easy enough to read function signatures if the expected return value is just a primitive, but if it needs to conform to an interface, then help is good.

By the way, it's time for me to point out the master list of all tracks' discussions on providing stubs exercism/discussions#114

I'll let y'all decide what y'all want to do with the other exercises, but for this one, how far should we go?

only provide the Subscription interface, and specify that addCallback should return something that conforms to it?

provide Subscription, and stubs for Cell, ComputeCell, InputCell? But be careful, because this could constrain the student's implementation; maybe they don't want to have those exact three classes (maybe they just want to have a single Cell class?

I'll start with the first option and see how it goes.

petertseng · 2017-01-22T08:19:31Z

exercises/react/src/example/kotlin/React.kt

+    }
+
+    inner class InputCell(initialValue: T) : Cell() {
+        override var value: T = initialValue


looks like http://kotlinlang.org/docs/reference/delegated-properties.html#observable could be used here, I'll consider it, though it's understandable enough without it.

(I'll probably not use it in the PR. Maybe in my submission to exercism.io!)

Yes, observables do seem like a great fit for this exercise - I wonder if there is a way to leverage the ObservableProperty as a super type. But what you currently have works, we should care more about if API driven by the tests make sense.

petertseng · 2017-02-04T19:15:39Z

If #30 is merged before this PR, please notify me of such, so that I will know to add @Ignore to all tests except the first before y'all merge this PR.

If this PR is merged before #30, please notify #30 of such, so that @Ignore may be added to react in #30 as well.

Unfortunately it does not make much sense for me to add @Ignore to this PR pre-emptively (before #30 is merged) since that would leave the repo in an inconsistent state if this PR were merged first (and I don't want to risk putting this repo in that state), so we should take one of the two above routes instead.

jtigger · 2017-02-05T20:36:06Z

@exercism/kotlin anyone available to give this PR a review?

sdavids13 · 2017-02-07T03:39:16Z

My apologies for leaving this hanging, I have been a bit preoccupied lately. I took a glance at the PR and thought it was overly complicated but that's because I didn't have an understanding of this particular exercise. I wanted to take a look at a couple of other tracks and was thinking of trying it out myself. Unfortunately I haven't had the time to do that yet - @jtigger do you have experience with this exercise and does it look reasonable to you?

I would like to merge in the @Ignore PR first since that is pretty straight-forward.

sdavids13

Thanks for the contribution. Let me know what you think about the various comments, if you can also add the @Ignore to all but the first test that would be appreciated.

sdavids13 · 2017-02-09T03:07:19Z

config.json

@@ -222,6 +222,11 @@
      "difficulty": 1,
      "slug": "change",
      "topics": []
+    },
+    {
+      "difficulty": 1,


The difficulty should probably be either 9 or 10 I would think relative to the other exercises.

difficulty: other tracks rated it:

csharp, fsharp: 9

ceylon: 10 (but note that was a unilateral decision by me, so doesn't provide the perspective of others)

crystal, go, nim: not rated at this time (1)

rust: 10 (but Rust only has four difficulty ratings, 1, 4, 7, and 10)

I'll rate 10 for this track since I used generics, which C# and F# didn't

sdavids13 · 2017-02-09T03:24:00Z

exercises/react/src/example/kotlin/React.kt

+class Reactor<T>() {
+    abstract inner class Cell {
+        abstract val value: T
+        internal val dependents = mutableListOf<ComputeCell>()


Yea, I'm likewise surprised that this error shows up for a protected variable when being accessed in a subclass' constructor, it is available in the subclass outside of the constructor though...

sdavids13 · 2017-02-09T03:25:06Z

exercises/react/src/example/kotlin/React.kt

+    abstract inner class Cell {
+        abstract val value: T
+        internal val dependents = mutableListOf<ComputeCell>()
+        internal fun eachDependent(f: (ComputeCell) -> Any) {


This is the same thing as dependents.forEach { .... }

forEach

embarrassing that I missed that!

sdavids13 · 2017-02-09T03:29:33Z

exercises/react/src/example/kotlin/React.kt

+    }
+
+    inner class InputCell(initialValue: T) : Cell() {
+        override var value: T = initialValue


Yes, observables do seem like a great fit for this exercise - I wonder if there is a way to leverage the ObservableProperty as a super type. But what you currently have works, we should care more about if API driven by the tests make sense.

sdavids13 · 2017-02-09T03:30:44Z

exercises/react/src/example/kotlin/React.kt

+        override var value: T = initialValue
+            set(newValue) {
+                field = newValue
+                eachDependent { it.propagate() }


As noted earlier you can just do: dependents.forEach { it.propagate() }

sdavids13 · 2017-02-09T03:37:46Z

exercises/react/src/example/kotlin/React.kt

+        constructor(c: Cell, f: (T) -> T) : this({ f(c.value) }) {
+            c.dependents.add(this)
+        }
+        constructor(c1: Cell, c2: Cell, f: (T, T) -> T) : this({ f(c1.value, c2.value) }) {


This constructor pattern is starting to scream for varargs, unfortunately I don't believe you can specify a vararg for lambda parameters. Is there a better way to perhaps go about these constructors for increased flexibility? Would passing a collection make more sense?

unfortunately I don't believe you can specify a vararg for lambda parameters

Yeah I didn't find one either, nor a way to make sure that the 1-cell form is always with a 1-argument lambda, a 2-cell form with a 2-argument lambda.

Is there a better way to perhaps go about these constructors for increased flexibility? Would passing a collection make more sense?

I believe this is possible. The disadvantage is that we lose a bit of compiler verification: Now someone can pass in two cells with a function that expects to get one value (it only reads the first value out of its collection of arguments). The advantage of course is more flexibility - we can imagine a compute cell that takes 3 cells, etc. even though it is not tested.

What I'm going to do is, if possible, I will make a separate commit that explores using a collection. We can decide whether to have that commit or not have it, when we see what it looks like.

OK, I have prepared what that looks like in the last commit, and we can decide whether it is better or worse to have it. Only disadvantage is as I mentioned the possibility of a caller mismatching the arities, giving one cell but expecting two values in the lambda, vice versa. Of course, since the test suite is in our control we know we haven't made that mistake, so the concern is only "what would happen if we used this as a library and gave it to other people to use, what kinds of mistakes might they make with it"

I personally like the varargs way slightly better.

I like the way the lambda expression looks the old way but I do agree the vararg route seems better from an implementation perspective. In Kotlin 1.1 they are introducing destructing in lambdas which allows something like this: listOf(1, 2, 3).map { a, b, c -> "$a $b $c" }. With that new feature we can re-introduce the better lambda readability without needing to change the method signature.

sdavids13 · 2017-02-09T03:42:44Z

exercises/react/src/example/kotlin/React.kt

+            c2.dependents.add(this)
+        }
+
+        fun addCallback(f: (T) -> Any): Subscription {


I really like what you did here returning back a Subscription. Do you think we should provide some initial getting started help by at least providing some interfaces?

sdavids13 · 2017-02-09T04:17:10Z

exercises/react/src/test/kotlin/ReactTest.kt

+    companion object {
+        @JvmStatic
+        @Parameterized.Parameters(name = "{index}: {0} + {1} + {2} = {3} {4}")
+        fun data() = listOf(


All of these parameters make it a bit difficult to read, perhaps something like this would be an improvement:

diff --git a/exercises/react/src/test/kotlin/ReactTest.kt b/exercises/react/src/test/kotlin/ReactTest.kt index 92cc9e0..a514b6b 100644 --- a/exercises/react/src/test/kotlin/ReactTest.kt +++ b/exercises/react/src/test/kotlin/ReactTest.kt @@ -169,28 +169,32 @@ class ReactTest { @RunWith(Parameterized::class) // This is a digital logic circuit called an adder: // https://en.wikipedia.org/wiki/Adder_(electronics) -class ReactAdderTest(val aval: Boolean, val bval: Boolean, val cin: Boolean, val expectedCout: Boolean, val expectedSum: Boolean) { +class ReactAdderTest(val input: Input, val expected: Expected) { + companion object { + data class Input(val a: Boolean, val b: Boolean, val carryIn: Boolean) + data class Expected(val carryOut: Boolean, val sum: Boolean) + @JvmStatic - @Parameterized.Parameters(name = "{index}: {0} + {1} + {2} = {3} {4}") + @Parameterized.Parameters(name = "{index}: {0} = {1}") fun data() = listOf( - arrayOf(false, false, false, false, false), - arrayOf(false, false, true, false, true), - arrayOf(false, true, false, false, true), - arrayOf(false, true, true, true, false), - arrayOf(true, false, false, false, true), - arrayOf(true, false, true, true, false), - arrayOf(true, true, false, true, false), - arrayOf(true, true, true, true, true) + arrayOf(Input(a=false, b=false, carryIn=false), Expected(carryOut=false, sum=false)), + arrayOf(Input(a=false, b=false, carryIn=true), Expected(carryOut=false, sum=true)), + arrayOf(Input(a=false, b=true, carryIn=false), Expected(carryOut=false, sum=true)), + arrayOf(Input(a=false, b=true, carryIn=true), Expected(carryOut=true, sum=false)), + arrayOf(Input(a=true, b=false, carryIn=false), Expected(carryOut=false, sum=true)), + arrayOf(Input(a=true, b=false, carryIn=true), Expected(carryOut=true, sum=false)), + arrayOf(Input(a=true, b=true, carryIn=false), Expected(carryOut=true, sum=false)), + arrayOf(Input(a=true, b=true, carryIn=true), Expected(carryOut=true, sum=true)) ) } @Test fun test() { val reactor = Reactor<Boolean>() - val a = reactor.InputCell(aval) - val b = reactor.InputCell(bval) - val carryIn = reactor.InputCell(cin) + val a = reactor.InputCell(input.a) + val b = reactor.InputCell(input.b) + val carryIn = reactor.InputCell(input.carryIn) val aXorB = reactor.ComputeCell(a, b) { a, b -> a.xor(b) } val sum = reactor.ComputeCell(aXorB, carryIn) { axorb, cin -> axorb.xor(cin) } @@ -199,10 +203,6 @@ class ReactAdderTest(val aval: Boolean, val bval: Boolean, val cin: Boolean, val val aAndB = reactor.ComputeCell(a, b) { a, b -> a && b } val carryOut = reactor.ComputeCell(aXorBAndCin, aAndB) { a, b -> a || b } - // Test them both at once so if they fail we get to see both values. - assertEquals( - listOf(expectedSum, expectedCout), - listOf(sum.value, carryOut.value) - ) + assertEquals(expected, Expected(sum=sum.value, carryOut=carryOut.value)) } }

All of these parameters make it a bit difficult to read, perhaps something like this would be an improvement

So, I hope you don't mind if I just take that exactly as-is.

Sounds good to me ;)

petertseng

Thanks for the review!

I'm adding changes as separate commits so reviewers can easily see what changed. It will probably be best to squash them all when merging. Or I can squash if that's what y'all want.

Still coming up are two items:

~~provide interface help~~
~~explore whether we can use a collection for the ComputeCell ctor~~

Both done.

petertseng · 2017-02-09T04:55:30Z

config.json

@@ -222,6 +222,11 @@
      "difficulty": 1,
      "slug": "change",
      "topics": []
+    },
+    {
+      "difficulty": 1,


difficulty: other tracks rated it:

csharp, fsharp: 9

ceylon: 10 (but note that was a unilateral decision by me, so doesn't provide the perspective of others)

crystal, go, nim: not rated at this time (1)

rust: 10 (but Rust only has four difficulty ratings, 1, 4, 7, and 10)

I'll rate 10 for this track since I used generics, which C# and F# didn't

petertseng · 2017-02-09T04:57:29Z

exercises/react/src/example/kotlin/React.kt

+    abstract inner class Cell {
+        abstract val value: T
+        internal val dependents = mutableListOf<ComputeCell>()
+        internal fun eachDependent(f: (ComputeCell) -> Any) {


forEach

embarrassing that I missed that!

petertseng · 2017-02-09T05:02:31Z

exercises/react/src/example/kotlin/React.kt

+        constructor(c: Cell, f: (T) -> T) : this({ f(c.value) }) {
+            c.dependents.add(this)
+        }
+        constructor(c1: Cell, c2: Cell, f: (T, T) -> T) : this({ f(c1.value, c2.value) }) {


unfortunately I don't believe you can specify a vararg for lambda parameters

Yeah I didn't find one either, nor a way to make sure that the 1-cell form is always with a 1-argument lambda, a 2-cell form with a 2-argument lambda.

Is there a better way to perhaps go about these constructors for increased flexibility? Would passing a collection make more sense?

I believe this is possible. The disadvantage is that we lose a bit of compiler verification: Now someone can pass in two cells with a function that expects to get one value (it only reads the first value out of its collection of arguments). The advantage of course is more flexibility - we can imagine a compute cell that takes 3 cells, etc. even though it is not tested.

What I'm going to do is, if possible, I will make a separate commit that explores using a collection. We can decide whether to have that commit or not have it, when we see what it looks like.

petertseng · 2017-02-09T05:06:31Z

exercises/react/src/example/kotlin/React.kt

+            c2.dependents.add(this)
+        }
+
+        fun addCallback(f: (T) -> Any): Subscription {


Do you think we should provide some initial getting started help by at least providing some interfaces?

I would be in support of that, because it may be otherwise difficult to determine from just the tests that addCallback is supposed to return something that you can call cancel() on. It's easy enough to read function signatures if the expected return value is just a primitive, but if it needs to conform to an interface, then help is good.

By the way, it's time for me to point out the master list of all tracks' discussions on providing stubs exercism/discussions#114

I'll let y'all decide what y'all want to do with the other exercises, but for this one, how far should we go?

only provide the Subscription interface, and specify that addCallback should return something that conforms to it?

provide Subscription, and stubs for Cell, ComputeCell, InputCell? But be careful, because this could constrain the student's implementation; maybe they don't want to have those exact three classes (maybe they just want to have a single Cell class?

I'll start with the first option and see how it goes.

petertseng · 2017-02-09T05:07:27Z

exercises/react/src/test/kotlin/ReactTest.kt

+    companion object {
+        @JvmStatic
+        @Parameterized.Parameters(name = "{index}: {0} + {1} + {2} = {3} {4}")
+        fun data() = listOf(


All of these parameters make it a bit difficult to read, perhaps something like this would be an improvement

So, I hope you don't mind if I just take that exactly as-is.

We show the student about addCallback returning a Subscription. .keep file is removed, since there is now a file in the directory.

Added in #30 to all exercises so I need to add it too.

petertseng · 2017-02-09T05:37:41Z

exercises/react/src/example/kotlin/React.kt

+        private var callbacksIssued = 0
+        private val activeCallbacks = mutableMapOf<Int, (T) -> Any>()
+
+        constructor(vararg cells: Cell, f: (List<T>) -> T) : this({ f(cells.map { it.value }) }) {


List seemed most appropriate since it allowed indexing into it, so the lambdas can say it[0], it[0] + it[1], etc. Using Iterable could work, but would be weird - we can do it.first and it.fold { a, b -> a + b} or something, I suppose.

petertseng · 2017-02-09T05:44:32Z

config.json

+    {
+      "difficulty": 10,
+      "slug": "react",
+      "topics": []


this track want to add any topics? I suggest a similar list as those in Ceylon https://github.com/exercism/xceylon/blob/master/config.json#L52

Reactive programming, generics, nested classes, higher-order functions?

Yea, would be great to fill them in where we can.

sdavids13 · 2017-02-10T03:34:55Z

This looks good to me, the only thing that is a little concerning is not having a more stubs available to point people in the correct syntax direction for some of the more complicated method signatures (such as the ComputeCell constructor). But we may be fine seeing how we ranked this the highest difficulty, does anyone know if there is any feedback mechanisms that may tip us if certain exercises are being more problematic than others?

petertseng · 2017-02-10T04:03:09Z

not having a more stubs available to point people in the correct syntax direction for some of the more complicated method signatures

This will be interesting, because it would be possible for a student to go one of two ways:

Define two separate constructors, one for the one-cell form and one for the two-cell form
Define a single constructor with varargs.

So it could give insight to see how students take advantage of the freedom given to them, in this case.

The other big potential stumbling block is the syntax for passing in a function as an argument. I actually imagine this can be alleviated based on the fact that this track has both accumulate and strain, which will require this.

any feedback mechanisms that may tip us if certain exercises are being more problematic than others?

Oh, often I wish for this too. exercism/discussions#86 mentions it . I don't think there is something implemented, and it may fold into the big user experience rethinking.

sdavids13 · 2017-02-10T05:30:33Z

Good points, I'm convinced :) Thanks for all the effort you put in this, it's a good challenge to throw at people.

petertseng · 2017-02-10T05:36:16Z

Thanks!

petertseng commented Jan 18, 2017

View reviewed changes

petertseng changed the title ~~declaration of intent to implement react. expiry: 2017-01-25~~ react: Add new exercise Jan 18, 2017

petertseng added 2 commits January 18, 2017 02:43

react: add new exercise

60adbe3

petertseng commented Jan 18, 2017

View reviewed changes

petertseng commented Jan 22, 2017

View reviewed changes

sdavids13 suggested changes Feb 9, 2017

View reviewed changes

Peter Tseng and others added 4 commits February 8, 2017 20:55

react: difficulty -> 10

e356e40

react: eachDepenedent -> dependents.forEach

c2b907e

react: cleanly separate expected and input values in adder test

cc174c5

react: ignore all but first test

75ccd07

petertseng commented Feb 9, 2017

View reviewed changes

Peter Tseng added 3 commits February 8, 2017 21:18

react: Add interface help

d5fcdcc

We show the student about addCallback returning a Subscription. .keep file is removed, since there is now a file in the directory.

react: Add testLogging to build.gradle

aeb4ea8

Added in #30 to all exercises so I need to add it too.

react: use varargs in ComputeCell ctor

75f14fa

petertseng commented Feb 9, 2017

View reviewed changes

react: add topics

58f3b19

jtigger changed the title ~~react: Add new exercise~~ react: add to track Feb 9, 2017

sdavids13 approved these changes Feb 10, 2017

View reviewed changes

sdavids13 merged commit cab34fe into exercism:master Feb 10, 2017

petertseng deleted the react branch February 10, 2017 05:36

sdavids13 mentioned this pull request Mar 2, 2017

React test lambda destructuring #39

Merged

react: add to track #29

react: add to track #29

Conversation

petertseng commented Jan 18, 2017 • edited Loading

petertseng Jan 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petertseng commented Jan 18, 2017 • edited Loading

petertseng Jan 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petertseng Jan 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petertseng commented Feb 4, 2017 • edited Loading

jtigger commented Feb 5, 2017

sdavids13 commented Feb 7, 2017

sdavids13 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petertseng Feb 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petertseng Feb 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petertseng Feb 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petertseng left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petertseng Feb 9, 2017 • edited Loading

Choose a reason for hiding this comment

petertseng Feb 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petertseng Feb 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdavids13 commented Feb 10, 2017

petertseng commented Feb 10, 2017

sdavids13 commented Feb 10, 2017

petertseng commented Feb 10, 2017

petertseng commented Jan 18, 2017 •

edited

Loading

petertseng Jan 18, 2017 •

edited

Loading

petertseng commented Jan 18, 2017 •

edited

Loading

petertseng Jan 18, 2017 •

edited

Loading

petertseng Jan 22, 2017 •

edited

Loading

petertseng commented Feb 4, 2017 •

edited

Loading

petertseng Feb 9, 2017 •

edited

Loading

petertseng Feb 9, 2017 •

edited

Loading

petertseng Feb 9, 2017 •

edited

Loading

petertseng left a comment •

edited

Loading

petertseng Feb 9, 2017 •

edited

Loading

petertseng Feb 9, 2017 •

edited

Loading

petertseng Feb 9, 2017 •

edited

Loading