Skip to content

Implement exercise Poker#347

Merged
petertseng merged 9 commits intoexercism:masterfrom
coriolinus:poker
Sep 19, 2017
Merged

Implement exercise Poker#347
petertseng merged 9 commits intoexercism:masterfrom
coriolinus:poker

Conversation

@coriolinus
Copy link
Copy Markdown
Member

Staking a claim on implementing the poker exercise.

coriolinus and others added 5 commits August 24, 2017 10:26
Update master to exercism/rust:master
Description and tests copied from
https://github.com/exercism/problem-specifications/tree/master/exercises/poker

A `lib.rs` stub is included so that users don't have to puzzle out the type signature.
* Ignore all but the first test
* Add entry to `config.json` and ensure `configlet lint` has no output

Tried running `configlet generate . --only poker`, but that eliminated all problem-specific
text from the README; reverted that change before commit.
@coriolinus coriolinus changed the title Implement exercise Poker (WIP) Implement exercise Poker Aug 24, 2017
@coriolinus
Copy link
Copy Markdown
Member Author

Travis passes, so I think this should be all set for someone with merge authority.

Note that the command configlet generate . --only poker eliminated all problem-specific text from the README, so either I was using it wrong or we'll want to override its output.

///
/// Note the type signature: this function should return _the same_ reference to
/// the winning hand(s) as were passed in, not reconstructed strings which happen to be equal.
pub fn winning_hands<'a>(_: &[&'a str]) -> Option<Vec<&'a str>> {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer using a variable name instead of _

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Any variable name we use will need to begin with an underscore anyway, to prevent compiler warnings about unused variables.
  2. Using _ encourages the implementer choose their own variable name.
  3. The only point of including the function stub is so that users don't have to guess and check the expected signature by inspecting the tests. While the name used is technically part of the public signature of the function, I figured it best to minimize the degree to which we constrain the individual implementers.

If you feel very strongly about it, I can change it, but I think there are good reasons to leave it as is.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I'm convinced.

@ijanos
Copy link
Copy Markdown
Contributor

ijanos commented Sep 3, 2017

Here is what I'm thinking about: I think the rust track would really benefit from an exercise that teaches implementing PartialOrd as implementing traits is a really important concept. I see two options:

  • modify this exercise to focus on the PartialOrd implementation.
  • leave this one as is and add a simple track specific exercise about PartialOrd somewhere before this one and modify the poker readme nudging people to use that knowledge to create the implementation for this one.

I think the second option would be better, what do you think?

@coriolinus
Copy link
Copy Markdown
Member Author

I'd agree that traits are an important concept to learn, and that this exercise is a pretty natural fit for using that trait implementation. After all, that's the path I went in my own solution. That said, I tend to prefer exercises which allow a broad range of implementations over those which railroad the implementer into a particular design choice, and the required function signature already requires that people know about lifetimes in function signatures.

In short, I agree that the second path is better. I'm going to adjust the readme now to suggest the use of PartialOrd without actually requiring it from the tests.

Possibly the simplest way to solve this problem is to create a PokerHand type
which implements PartialOrd, and then sort the input and take the highest values.
However, this may not be obvious to novice coders. The new hints in the readme
should point people in the right direction.
@ijanos
Copy link
Copy Markdown
Contributor

ijanos commented Sep 5, 2017

This is awesome, thank you! I will wait a few days with the merge to give a chance to other maintainers to chime in, if you don't mind.

@coriolinus
Copy link
Copy Markdown
Member Author

@ijanos It's been two weeks with no further comments; perhaps you could merge this now?

Copy link
Copy Markdown
Member

@petertseng petertseng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I confirm that I have no* additional comments to make.

*: Well, OK, some, but these are about things other than the exercise; they were about the README and config.json

Comment thread config.json
"uuid": "0a33f3ac-cedd-4a40-a132-9d044b0e9977",
"slug": "poker",
"core": false,
"unlocked_by": null,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this will be our first exercise of difficulty 6.

Historically, this track has only used difficulties 1, 4, 7, and 10, corresponding to the four sections (Introduction = 1, Getting Rusty = 4, Rust Gets Strange = 7, Putting it all Together = 10) in https://github.com/exercism/rust/blob/aea470a76a9c6f0005adebb2a00d969512da3b6e/problems.md. That file does not exist anymore as it has been replaced with https://github.com/exercism/rust/blob/master/problem_ordering.md . So we are not restricted to only doing 1 4 7 10. But you would be breaking new ground.

I am assuming all parties involved are OK with this. I'm also inferring that everyone agreed that this is harder than all exercises of difficulty 4 and easier than all exercises of difficulty 7.

This comment therefore does not require a response or anything to be changed, but I was obligated to write this comment for the sake of anyone who looks back on this PR and wonders about the origin of the first difficulty 6 exercise.

Comment thread exercises/poker/README.md

## Hints

- Ranking a list of poker hands can be considered a sorting problem.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these hints are Rust-specific, therefore for the README generator to not remove them, they need to be placed in .meta/hints.md . I did so in a commit.

@petertseng
Copy link
Copy Markdown
Member

petertseng commented Sep 18, 2017

Question: README generator writes the description wrapped to some number of columns, because those are how the files appear in https://github.com/exercism/problem-specifications/blob/master/exercises/poker/description.md / https://raw.githubusercontent.com/exercism/problem-specifications/master/exercises/poker/description.md and https://github.com/exercism/rust/blob/master/config/exercise-readme-insert.md / https://raw.githubusercontent.com/exercism/rust/master/config/exercise-readme-insert.md .

So, regenerating the README results in this diff:

f4b2ae5

I'm wondering whether the hints should get wrapped too, to match.

Comment thread exercises/poker/README.md Outdated

## Source

J Dalbey's Programming Practice problems <http://users.csc.calpoly.edu/~jdalbey/103/Projects/ProgrammingPractice.html>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@petertseng
Copy link
Copy Markdown
Member

Well, my constraints are that I don't want to maintain any README file that some generator does not generate (even if it isn't configlet's generator), so I would prefer to use the README as it was appears in f4b2ae5 , hoping nobody has objections to that (the fact that some lines are wrapped and some aren't). Assuming nobody has complaints, then there is nothing more to be said

@petertseng petertseng merged commit 207aee1 into exercism:master Sep 19, 2017
@petertseng
Copy link
Copy Markdown
Member

Thanks for implementing this exercise!

@coriolinus coriolinus deleted the poker branch September 20, 2017 17:18
petertseng pushed a commit that referenced this pull request Oct 12, 2017
Per the comment [here](#347 (comment)),
there's continued interest in a Rust-specific exercise focusing on
teaching trait implementation.

My idea is to have users implement an arbitrary-precision `Decimal`
type[1], and ensure that standard math is possible for them. I.e. they
must implement at least `Add`, `Sub`, `Mul`, `PartialEq`, and 
`PartialOrd`; tests will ensure that the `==`, `+`, `-`, `*`, `>`, and 
`<` operators all work as expected.

---
[1] We'd have to ask people not to simply import
[bigdecimal](https://crates.io/crates/bigdecimal), which does exactly
what we require.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants