Add canonical-data for poker #793

arthurchipdean · 2017-05-17T02:22:25Z

closes #576

petertseng

thanks.

since this closes #576, I must edit your PR description to say "closes #576", not just "#576".

petertseng · 2017-05-17T04:42:04Z

exercises/poker/canonical-data.json

+          "property": "best_hand",
+          "input": [
+            "2S 8H 2D 8D 3H",
+            "44S 5H 4C 8S 5D"


petertseng · 2017-05-17T04:42:19Z

exercises/poker/canonical-data.json

+          "input": [
+            "4S 5S 7H 8D JC"
+          ],
+          "expected": "4S 5S 7H 8D JC"


the value at expected should be an array, to match all the rest

petertseng · 2017-05-17T04:42:23Z

exercises/poker/canonical-data.json

+            "2S 4C 7S 9H 10H",
+            "3S 4S 5D 6H JH"
+          ],
+          "expected": "3S 4S 5D 6H JH"


the value at expected should be an array, to match all the rest

petertseng · 2017-05-17T04:42:32Z

exercises/poker/canonical-data.json

+          ]
+        }
+      ]
+    }]


any reason for } and ] on same line?

petertseng · 2017-05-17T04:43:49Z

exercises/poker/canonical-data.json

+          ]
+        },
+        {
+          "description": "aces can be end a straight (10 J Q K A)",


I think either remove "be" or add "of"

petertseng · 2017-05-17T04:43:54Z

exercises/poker/canonical-data.json

+          ]
+        },
+        {
+          "description": "aces can be start a straight (A 2 3 4 5)",


I think either remove "be" or add "of"

petertseng · 2017-05-17T04:46:10Z

exercises/poker/canonical-data.json

+      "cases": [
+        {
+          "description": "single hand always wins",
+          "property": "best_hand",


_ isn't an allowed character in https://github.com/exercism/x-common/blob/master/canonical-schema.json#L117, so I would suggest bestHand. Maybe even bestHands since there can be more than one.

petertseng · 2017-05-17T04:47:19Z

exercises/poker/canonical-data.json

+  "cases": [
+    {
+      "description": "it should be able to pick the winning poker hand",
+      "cases": [


I'm not convinced a second level of cases nesting is necessary. I suggest just a single level, please, unless there is a specific reason to use two, and it doesn't seem that the extra description here adds any benefit. For example, https://github.com/exercism/x-common/blob/master/exercises/hello-world/canonical-data.json uses just one level.

petertseng · 2017-05-17T05:12:47Z

Of the existing implementations, I'm not going to check which of them have which cases, but I will check for the sake of interest:

which of them represent the hands as a single string, and which represent them as a list of strings?
which of them use SDCH as their suits, and which use ♤♢♧♡?
which of them use 10 as their ten, and which use T?

I don't think any of these need to inform decisions in this PR, but they can if y'all want.

We probably can't do ♤♢♧♡ since they violate Discussion: Non-Ascii test cases. #428.
I prefer 10 as the ten, but I really don't care.

test	hand type	suits	tens
https://github.com/exercism/xcsharp/blob/master/exercises/poker/PokerTest.cs	one string	SDCH	T
https://github.com/exercism/xdelphi/blob/master/exercises/poker/uPokerTest.pas	one string	SDCH	T
https://github.com/exercism/xelixir/blob/master/exercises/poker/poker_test.exs	list of strings (`~w`)	SDCH	10
https://github.com/exercism/xfsharp/blob/master/exercises/poker/PokerTest.fs	one string	SDCH	T
https://github.com/exercism/xgo/blob/master/exercises/poker/poker_test.go	one string	♤♢♧♡	10
https://github.com/exercism/xpython/blob/master/exercises/poker/poker_test.py	list of strings (`split`)	SDCH	T
https://github.com/exercism/xruby/blob/master/exercises/poker/poker_test.rb	list of strings (`%w`)	SDCH	10
https://github.com/exercism/xswift/blob/master/exercises/poker/Tests/PokerTests/PokerTests.swift	one string	♤♢♧♡	10

(Go and Swift test various unparsable hands)

petertseng · 2017-05-17T05:13:23Z

https://github.com/petertseng/x-common/blob/verify-poker/exercises/poker/verify.rb says all cases here are correct except for the 44S.

arthurchipdean · 2017-05-17T05:15:19Z

I copied the test cases from the elixir exercise. I added an additional level of tests because I thought that was the reason it was failing the travis ci originally. I made all the changes you mentioned. I'm just trying to get this setup so that I can implement it in a standardized way in the ECMAScript exercise track.. Thanks!

petertseng · 2017-05-17T05:17:01Z

https://github.com/petertseng/x-common/tree/verify-poker/exercises/poker has been updated and it says all cases are correct

petertseng

potential case to add: two hands that each have the same pair, to exercise comparison of the other cards

Since for every other kind of hand X, there is a case for "two hands that each have the same X"

petertseng · 2017-05-17T05:19:32Z

exercises/poker/canonical-data.json

+      ]
+    },
+    {
+      "description":"aces can end a straight (10 J Q K A)",


missing a space after the colon (consistency with other cases)

I believe that case is already taken care of by this test: "both hands have two identically ranked pairs, tie goes to remaining card (kicker)".

potential case to add: two hands that each have the same pair, to exercise comparison of the other cards

I believe that case is already taken care of by this test: "both hands have two identically ranked pairs, tie goes to remaining card (kicker)".

Interesting. I hadn't thought of it that way, since that was a two-pair hand rather than a one-pair hand, but I can see it working out.

OK, just that extra space

... which you just put in, ok.

stevejb71 · 2017-05-17T14:54:01Z

exercises/poker/canonical-data.json

+      ]
+    },
+    {
+      "description": "with multiple decks, both hands with identical four of a kind, tie determined by kicker" ,


I've not heard the term kicker before, it may be poker jargon which not everyone will understand.

I'd say the term could be easily understood by the context. Look at the test data, the meaning is ridiculously obvious. Plus the word kicker is actually in the dictionary as a poker term so if they opened a dictionary they would also get the answer. If we start dumbing down the description, where do we draw the line? Should I limit my word choices to a certain reading level?

@stevejb71 Also the bowling test data contains words like Turkey, Double, etc... Are you saying we should change these to say: 3 strikes in a row, 2 strikes in a row. It seems very silly.

the bowling test data contains words like Turkey, Double, etc...

errr? https://github.com/search?q=org%3Aexercism+turkey&type=Code

and just in case that missed something, nothing in https://github.com/exercism/x-common/blob/master/exercises/bowling/canonical-data.json either.

???

confused

Do you know though, in this case the problem is solved: "kicker" is defined earlier #793 (comment)

Therefore, this description is allowed to rely on the earlier definition!

@petertseng Even if the word turkey is not being used other bowling jargon like Strike and Spare are both being used. My point is still valid. The actual word being used is irrelevant, the point is that bowling jargon is being used.

Strike and Spare are both being used

Thanks. I see they are defined at https://github.com/exercism/x-common/blob/master/exercises/bowling/description.md .

So in the same way one would ask is it OK to have "straight", "flush", "full house"? (Would anyone have trouble understanding what "pair" means?) The https://github.com/exercism/x-common/blob/master/exercises/poker/description.md incorporates https://en.wikipedia.org/wiki/List_of_poker_hands by reference, can there be any room for doubt there?

I would argue we have done what we need to do - the terms are defined in the README or earlier in the file, right?

If there is any case where we failed to do that, in this exercise or any other, it seems good to correct it.

I agree. Thanks for clarifying. :)

petertseng · 2017-05-20T00:00:00Z

exercises/poker/canonical-data.json

+      ]
+    },
+    {
+      "description": "both hands have two identically ranked pairs, tie goes to remaining card (kicker)",


it is defined, which solves the problem of "I might not know what a kicker is"

arthurchipdean · 2017-05-20T00:45:32Z

I have fixed all formatting and other issues. Please let me know if there is anything else I need to do.

Thank you!

petertseng

From my point of view, everything is in order. So if nobody else has anything to say by the time the weekend ends let's merge.

Insti · 2017-05-20T06:59:37Z

I strongly prefer T for 10
I would probably have a test for comparing 2 hands as the second test rather than jumping straight to 3 hands.
I would group the example hands in a usefully human readable way that showed the correct ranking from left to right.
eg:

      "description": "highest pair wins",
      "property": "bestHands",
      "input": [
        "2H 2D JH 6S 4S",
        "4H 4D JD 6C 2D"

Then add further tests at the end that tested that everything worked with unordered hands.

But none of these should be blockers, everything else looks fine.

petertseng · 2017-05-22T00:49:08Z

Looks like it's time to merge, with thanks!

I agree that the non-blocking comments are non-blocking; I'll say what I think about them!

petertseng · 2017-05-22T01:14:55Z

T for 10

Tough choice, though I admit T has the advantage of taking up the same amount of space... but 10 is what you would see on an actual playing card. No way to please everyone here. Best I can think of is that tracks can convert in their generators as necessary. In the most extreme case, we could represent cards in JSON as {"rank": 10, "suit": "spades"} and let the tracks decide how to represent each hand, but it seems a bit too verbose to be my current suggestion.

2 hands as the second test rather than jumping straight to 3 hands

This one's interesting, wondering why it wasn't deemed blocking. For me, I'm not the kind of person who likes to write code that would only compare two hands only to have to replace it later; I would just make it work right from the beginning. So I guess that's why I deemed it non-blocking. Any other reason to take into account? Is it easier to debug if there are only two hands? That would be a reason to want that case.

human readable way that showed the correct ranking from left to right
Then add further tests at the end that tested that everything worked with unordered hands.

That one is interesting too! It would help readability of the test cases, though perhaps the description already helps here. I would become slightly less confident that the code under test can handle any order... though I guess I have unknown confidence right now since I don't know if there's any rhyme or reason to the orders right now. I'm slightly worried that the number of tests will increase a lot if we have to make sure that every kind of hand can be recognised in an arbitrary order, but I guess it's hard to know without seeing a concrete proposal. I suppose if someone wants to work on it and propose it that person can say so (sorry, not going to be that person)

arthurchipdean added 2 commits May 16, 2017 19:21

Add canonical-data for poker

6fc4ef7

Add additional level to cases for canonical-data

e82aecc

petertseng reviewed May 17, 2017

View reviewed changes

Fix json formatting.

ab827ec

petertseng reviewed May 17, 2017

View reviewed changes

stevejb71 reviewed May 17, 2017

View reviewed changes

petertseng reviewed May 20, 2017

View reviewed changes

Add space after colon to fix formatting

bad9ff7

petertseng approved these changes May 20, 2017

View reviewed changes

petertseng merged commit d078ba8 into exercism:master May 22, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add canonical-data for poker #793

Add canonical-data for poker #793

arthurchipdean commented May 17, 2017 •

edited by petertseng

Loading

petertseng left a comment

petertseng May 17, 2017

petertseng May 17, 2017

petertseng May 17, 2017

petertseng May 17, 2017

petertseng May 17, 2017

petertseng May 17, 2017

petertseng May 17, 2017

petertseng May 17, 2017

petertseng commented May 17, 2017

petertseng commented May 17, 2017

arthurchipdean commented May 17, 2017

petertseng commented May 17, 2017

petertseng left a comment •

edited

Loading

petertseng May 17, 2017

arthurchipdean May 20, 2017

petertseng May 20, 2017

stevejb71 May 17, 2017

arthurchipdean May 19, 2017

arthurchipdean May 19, 2017

petertseng May 20, 2017 •

edited

Loading

arthurchipdean May 20, 2017 •

edited

Loading

petertseng May 20, 2017 •

edited

Loading

arthurchipdean May 20, 2017

petertseng May 20, 2017 •

edited

Loading

arthurchipdean commented May 20, 2017

petertseng left a comment

Insti commented May 20, 2017 •

edited

Loading

petertseng commented May 22, 2017

petertseng commented May 22, 2017

Add canonical-data for poker #793

Add canonical-data for poker #793

Conversation

arthurchipdean commented May 17, 2017 • edited by petertseng Loading

petertseng 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

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 commented May 17, 2017

petertseng commented May 17, 2017

arthurchipdean commented May 17, 2017

petertseng commented May 17, 2017

petertseng left a comment • 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

Choose a reason for hiding this comment

petertseng May 20, 2017 • edited Loading

Choose a reason for hiding this comment

arthurchipdean May 20, 2017 • edited Loading

Choose a reason for hiding this comment

petertseng May 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petertseng May 20, 2017 • edited Loading

Choose a reason for hiding this comment

arthurchipdean commented May 20, 2017

petertseng left a comment

Choose a reason for hiding this comment

Insti commented May 20, 2017 • edited Loading

petertseng commented May 22, 2017

petertseng commented May 22, 2017

arthurchipdean commented May 17, 2017 •

edited by petertseng

Loading

petertseng left a comment •

edited

Loading

petertseng May 20, 2017 •

edited

Loading

arthurchipdean May 20, 2017 •

edited

Loading

petertseng May 20, 2017 •

edited

Loading

petertseng May 20, 2017 •

edited

Loading

Insti commented May 20, 2017 •

edited

Loading