dominoes: Add generator and example #484

petertseng · 2016-11-08T10:19:30Z

With my apologies to #424, but @seanreed1111 claimed it was OK to let someone else take it.

This PR complies with the new names in #463.

Note that this exercise appears to differ from other xruby exercises in
the following significant manner:

Verification that the student function under test has produced a correct
answer is NOT done by comparing the observed output to an expected
output (or even a list of expected outputs), because the set of
acceptable outputs is quite large even for a small input set.

Thus, verification is done by running the observed output against a
function that verifies various properties about the output.

https://github.com/exercism/x-common/blob/master/exercises/dominoes/canonical-data.json
explains the approach taken.

petertseng · 2016-11-08T10:22:46Z

lib/dominoes_cases.rb

+    <<-WL.chomp
+input_dominoes = #{input}
+    output_chain = Dominoes.chain(input_dominoes)
+    #{can_chain ? 'assert_correct_chain(input_dominoes, output_chain)' : 'assert_equal nil, output_chain, "There should be no chain for #{input_dominoes}"'}


Line is uncomfortably long. Any way to shorten?

Note that it is not so long in the generated output, since only one of two alternatives is taken.

petertseng · 2016-11-08T10:24:09Z

exercises/dominoes/dominoes_test.rb

+    skip
+    input_dominoes = [[1, 2]]
+    output_chain = Dominoes.chain(input_dominoes)
+    assert_equal nil, output_chain, "There should be no chain for #{input_dominoes}"


try assert_nil

petertseng · 2016-11-08T10:32:32Z

exercises/dominoes/example.rb

+    }
+
+    # Found no suitable chain.
+    nil


unfortunately, it's going to have to be nil and not [].

Why? Because for the input Dominoes.chain([]), [] is the valid answer. Therefore, returning [] when you have no chain will mean that the answer [] is ambiguous.

It might be useful to add this in a comment in the example.rb

Sure. I'll note that that comment was partially targeting #469 (comment) to explain why in this case it cannot be that this method always returns the same object - it has to sometimes return nil instead of list. It's useful to have the comment somewhere, so example it will be. Putting it in the test file didn't seem right since it wasn't relevant to all students.

petertseng · 2016-11-08T10:35:46Z

Oh yeah, I was going to mention I did this to test out the instructions for writing a generator. Seems to work OK. I'd mention that you need to touch .version or the generator won't run (increment_version requires that such a file exists). And you need to have an example file too. I tried to generate my tests before working on my example, but increment_version_in_example requires that the example actually exist.

Note that this exercise appears to differ from other xruby exercises in the following significant manner: Verification that the student function under test has produced a correct answer is NOT done by comparing the observed output to an expected output (or even a list of expected outputs), because the set of acceptable outputs is quite large even for a small input set. Thus, verification is done by running the observed output against a function that verifies various properties about the output. https://github.com/exercism/x-common/blob/master/exercises/dominoes/canonical-data.json explains the approach taken.

Insti · 2016-11-08T12:11:26Z

exercises/dominoes/example.tt

+# Test data version:
+# <%= sha1 %>
+class DominoesTest < Minitest::Test
+  def assert_correct_chain(input_dominoes, output_chain)


Could you move this to the bottom of the class please.

In real life I'd put it at the top, but here it would be good if the first thing the student saw was the first test they need to implement.

Yeah that's probably better. I imagine the student might wonder "what's assert_correct_chain", but hopefully they can just search the file!

Insti · 2016-11-08T12:12:53Z

exercises/dominoes/dominoes_test.rb

+    input_normal = input_dominoes.map(&:sort).sort
+    output_normal = output_chain.map(&:sort).sort
+    assert_equal input_normal, output_normal,
+      'Dominoes used in the output must be the same as the ones given in the input'


I'd extract this test.

Insti · 2016-11-08T12:15:04Z

exercises/dominoes/dominoes_test.rb

+    output_chain.each_cons(2).with_index { |(d1, d2), i|
+      _, d1_right = d1
+      d2_left, _ = d2
+      assert_equal d1_right, d2_left,


What about using .first and .last here instead of _right and _left?

Yeah seems good. I got too paranoid with thinking people would submit lists with length != 2, but I realize now that the same_dominoes check catches that.

Insti · 2016-11-08T12:18:18Z

exercises/dominoes/dominoes_test.rb

+        "In chain #{output_chain}, right end of domino #{i} (#{d1}) and left end of domino #{i + 1} (#{d2}) must match"
+    }
+
+    return if output_chain.empty?


Move this up above line 17. If we're short-circuiting short circuit as early as possible.

Insti · 2016-11-08T12:18:53Z

exercises/dominoes/dominoes_test.rb

+      d2_left, _ = d2
+      assert_equal d1_right, d2_left,
+        "In chain #{output_chain}, right end of domino #{i} (#{d1}) and left end of domino #{i + 1} (#{d2}) must match"
+    }


I'd extract this too.

Insti · 2016-11-08T12:19:10Z

exercises/dominoes/dominoes_test.rb

+    d1_left, _ = first_domino
+    _, d2_right = last_domino
+    assert_equal d1_left, d2_right,
+      "In chain #{output_chain}, left end of first domino (#{first_domino}) and right end of last domino (#{last_domino}) must match"


Again extract.

I am assuming since made it a separate comment that the "consecutive" test and the "dominoes at the ends" should be in two separate methods instead of the same. I'll go with that for now, it's easy to bring them together if I was mistaken in my interpretation.

Insti · 2016-11-08T12:23:59Z

I did this to test out the instructions for writing a generator.

Great, thanks. (We're working on improving/simplifying the generator creation process.)

Insti · 2016-11-08T12:28:45Z

Thanks for doing this @petertseng
& Great commit message ❤️

Insti · 2016-11-08T12:29:22Z

Missclicked close 😿

petertseng · 2016-11-08T17:29:38Z

TODO list for me so I understand what feedback I've addressed.

explain in code comment why nil and not [] is the appropriate answer for "no chain possible"
move assert_correct_chain to bottom
extract "same dominoes" check.
extract "consecutive dominoes" check.
extract "dominoes at the ends" check.
in assert_correct_chain, short-circuit before the each_cons check.
first and last instead of _, d1_right

the old sha was on a personal branch and it would just confuse people if it were kept in this file. this one is on master

No need to worry about list lengths != 2. assert_same_dominoes has got it covered.

petertseng · 2016-11-08T17:54:21Z

Review note: These commits are meant to be squashed. The only reason they are currently separate is for ease of understanding what chaged.

petertseng · 2016-11-08T18:00:31Z

exercises/dominoes/dominoes_test.rb

+    assert_correct_chain(input_dominoes, output_chain)
+  end
+
+  def test_can_t_be_chained


slightly ugly name. Should it be cant instead? Can be done without too much pain (.tr("'", '') the test name instead of replace with underscore)`

petertseng · 2016-11-08T18:00:52Z

exercises/dominoes/dominoes_test.rb

+    assert_correct_chain(input_dominoes, output_chain)
+  end
+
+  def test_singleton_that_can_t_be_chained


another ugly can_t

Insti · 2016-11-08T18:27:29Z

lib/dominoes_cases.rb

@@ -1,6 +1,6 @@
 class DominoesCase < OpenStruct
  def test_name
-    'test_%s' % description.gsub(/['= -]+/, '_')
+    'test_%s' % description.tr("'", '').gsub(/[= -]+/, '_')


or even better: gsub(/can't/,'can not')

I like it, suggestion taken.

Insti · 2016-11-08T18:40:16Z

🎉 Thanks @petertseng
I wish I could leave reaction icons on pull requests.

Insti · 2016-11-08T18:44:46Z

exercises/dominoes/dominoes_test.rb

+    skip
+    input_dominoes = [[1, 2]]
+    output_chain = Dominoes.chain(input_dominoes)
+    assert_nil output_chain, "There should be no chain for #{input_dominoes}"


What do you think about making a helper method refute_correct_chain?
I'm not entirely sure it's a good idea, but I like the symmetry of all the tests using similar terminology.

From the point of view of the student looking at this test generator: There is a lot of repetitive "There should be no chain for #{input_dominoes}". Moving it into refute_correct_chain removes this repetition. So that's one point in favour of it. One point against is that it'd be very simple, but that's very minor.

Here is another proposal that might also go along with refute_correct_chain:

Should output_chain = Dominoes.chain(input_dominoes) go inside assert_correct_chain and refute_correct_chain? Again it would reduce duplication, and then those two methods would only have to take in the input_dominoes. Though this might make it harder for students to understand what they're supposed to implement (they have to look for the definitions of {assert,refute}_correct_chain).

So currently: I am thinking yes to refute_correct_chain and no to "move output_chain = Dominoes.chain(input_dominoes) into{assert,refute}_correct_chain`"

I am thinking yes to refute_correct_chain and no to "move output_chain = Dominoes.chain(input_dominoes) into{assert,refute}_correct_chain`"

I agree with this.
It is important not to hide the subject of the test.

Insti · 2016-11-08T18:54:33Z

exercises/dominoes/dominoes_test.rb

+require_relative 'dominoes'
+
+# Test data version:
+# 82eb00d


Another minor minor thing, I've started putting these on 1 line.

Ah, okay. One line it is. I think putting it on a separate line was an artifact of the time when both the sha and the git commit summary was included... nowadays since it's just the sha it probably makes sense on one line.

petertseng · 2016-11-08T18:56:18Z

lib/dominoes_cases.rb

+    <<-WL.chomp
+input_dominoes = #{input}
+    output_chain = Dominoes.chain(input_dominoes)
+    #{can_chain ? 'assert_correct_chain(input_dominoes, output_chain)' : 'assert_nil output_chain, "There should be no chain for #{input_dominoes}"'}


continuing a conversation in #484 (comment) about whether we should have refute_correct_chain

Yet another argument for:

There's a bit of asymmetry in this line.

There's a slight bit more logic in assert_nil output_chain, "There should be no chain for #{input_dominoes}" versus assert_correct_chain(input_dominoes, output_chain) where all the logic is offloaded to the example.tt file. I think it makes more sense to be consistent, so I have added refute_correct_chain. If we change our minds about wanting it we just remove the commit.

Insti · 2016-11-08T18:56:27Z

Once this is ready to go I'll try solving it* and see how the tests work in practice.

* Maybe not today.

The candidates are the domino or the domino flipped. We can reverse to flip the domino! It's just a list!

Insti · 2016-11-08T19:20:26Z

exercises/dominoes/dominoes_test.rb

+    skip
+    assert_equal 1, BookKeeping::VERSION
+  end
+


Would it be worth inserting the comment about why we can't just use input/output tests in between the bookkeeping and the custom asserts?

Seems reasonable. Added.

Insti

I think this looks good to go.

I've not done the exercise myself yet, but will do when I get a chance in the next few days.

Thanks for all your quick responses @petertseng ❤️

petertseng · 2016-11-09T02:09:57Z

👍

Note! Starting in three days from now my availability will be limited. Of course, if everything is fine, I am sure the squash button will do just fine, I think it'll only be necessary to keep the first commit message.
If something needs my attention after that time, be prepared for responses that are slower than they were today.

petertseng force-pushed the dominoes branch from add7611 to 02eb86e Compare November 8, 2016 10:21

petertseng commented Nov 8, 2016

View reviewed changes

petertseng force-pushed the dominoes branch 3 times, most recently from fd0f73f to b04e14c Compare November 8, 2016 10:31

petertseng commented Nov 8, 2016

View reviewed changes

petertseng force-pushed the dominoes branch from b04e14c to 0073eb4 Compare November 8, 2016 10:36

Insti mentioned this pull request Nov 8, 2016

[WIP] add dominoes #424

Closed

Insti reviewed Nov 8, 2016

View reviewed changes

Insti closed this Nov 8, 2016

Insti reopened this Nov 8, 2016

Insti added the in progress label Nov 8, 2016

Insti mentioned this pull request Nov 8, 2016

Generator improvements. #485

Closed

petertseng added 5 commits November 8, 2016 09:40

dominoes: Explain why nil for "no chain possible"

1ece052

dominoes: use an actual x-common sha

f271187

the old sha was on a personal branch and it would just confuse people if it were kept in this file. this one is on master

dominoes: Move assert_correct_chain to bottom

bf41baa

dominoes: Extract methods and give them names

4d082ee

dominoes: short-circuit earlier

aed667c

dominoes: Use first and last instead of destructure

43aa6f8

No need to worry about list lengths != 2. assert_same_dominoes has got it covered.

petertseng commented Nov 8, 2016

View reviewed changes

dominoes: change can_t to cant

644efa5

Insti reviewed Nov 8, 2016

View reviewed changes

petertseng added 2 commits November 8, 2016 10:36

dominoes: can't -> can not

aa1307f

dominoes: cant -> can_not

4ad379d

Insti reviewed Nov 8, 2016

View reviewed changes

dominoes: refute_correct_chain

d5571b8

petertseng commented Nov 8, 2016

View reviewed changes

petertseng added 2 commits November 8, 2016 10:57

dominoes: sha one line

9d2448c

dominoes: de-duplicate example

abd83db

The candidates are the domino or the domino flipped. We can reverse to flip the domino! It's just a list!

Insti reviewed Nov 8, 2016

View reviewed changes

dominoes: Explain why we have the asserts, not examples

eabc138

Insti approved these changes Nov 8, 2016

View reviewed changes

Insti merged commit 1940663 into exercism:master Nov 21, 2016

petertseng deleted the dominoes branch November 21, 2016 15:26

Insti removed the in progress label Nov 27, 2016

dominoes: Add generator and example #484

dominoes: Add generator and example #484

Conversation

petertseng commented Nov 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petertseng Nov 8, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petertseng commented Nov 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Insti Nov 8, 2016 • 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

Choose a reason for hiding this comment

Insti commented Nov 8, 2016 • edited Loading

Insti commented Nov 8, 2016

Insti commented Nov 8, 2016

petertseng commented Nov 8, 2016 • edited Loading

petertseng commented Nov 8, 2016

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

Insti commented Nov 8, 2016

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

Choose a reason for hiding this comment

Insti commented Nov 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Insti left a comment

Choose a reason for hiding this comment

petertseng commented Nov 9, 2016

petertseng Nov 8, 2016 •

edited

Loading

Insti Nov 8, 2016 •

edited

Loading

Insti commented Nov 8, 2016 •

edited

Loading

petertseng commented Nov 8, 2016 •

edited

Loading