Tests for scrabble-score's optional extensions #249

juleskers · 2017-01-19T21:05:03Z

So I set myself the challenge of doing scrabble-score's extensions for :double- and :triple-scoring letters.
In the course of doing so, I wrote tests. I thought these may be useful to the source, so here is my offer to include them upstream.

Comments, suggestions, nits and other (constructive) criticism welcome; this is my first contribution to exercism.io, so I may need some guidance ;-)

…iple

juleskers · 2017-01-20T11:33:40Z

Aha, the Travis tests fail because my new tests fail.
It seems the reference implementation doesn't actually do the double and triple letter scoring.

What is the preferred method for me to continue?
Should I rewrite the example.rs to also handle this behaviour?

The rewriting would be non-trivial, because it requires state across individual characters. The current .fold() implementation can't be extended in that direction.
I could of course paste in my own solution[1][2], but I'm not sure that is as elegant yet as the "gold standard" example should be.

my solution:
[1] on github
[2] exercism submission

petertseng

Hello and thank you.

It is true that we would like the example solution to pass the tests - this helps us ensure the exercise is actually solvable (we didn't make a mistake in the tests, or something).

I don't mind your solution. The match for scoring each word is at least more compact than tan insert per letter, at least in terms of numbers of lines.

If you really want to fit double/triple into a fold, you would have the accumulator of the fold keep track of the previous letter's score as well, this would also be a worthy alternative.

I will wonder whether we should error on things like a:garbage or :double (doubling with no letter to double?), and also whether :double is the best way to notate that the letter should be doubled (then again, it's not like I can come up with something better; maybe [double] or something)

Oh yeah! and before I forget, I have to ask to ignore all but the first test, as I did in the other PR

IanWhitney · 2017-01-21T13:41:02Z

I think this is a good start. Thanks.

juleskers · 2017-01-21T21:28:10Z

Thanks for the feedback! I'll try to incorporate it!
The #[ignore] will be added, but probably only after I've passed the Travis round at least once.
(Or is travis smart enough to un-ignore the tests?)

I may be slow to respond, but that won't mean I've abandoned this, it's just that I'm doing this in the brief lost minutes during my commute :-)

I'm undecided on :garbage;
I was considering that :doubleSomeword would be the syntax for a double-scoring word; The spec leaves open how this would be expected.

petertseng · 2017-01-21T21:56:42Z

(Or is travis smart enough to un-ignore the tests?)

Yes, it does. I should have explained it, my bad for not doing so!
You don't necessarily need to look at this script, but if you are curious, our Travis CI runs https://github.com/exercism/xrust/blob/master/_test/check-exercises.sh#L42, thus unignoring all tests.

I was considering that :doubleSomeword would be the syntax for a double-scoring word; The spec leaves open how this would be expected.

Ah! This could be a fine idea. You are right about the spec.

As in the other PR, this could be a thing all languages can benefit from, so it could be worth opening an issue in x-common to discuss how the spec should be interpreted. And if we reach an agreement, a PR can be sent to x-common.

If agreement can't be reached in x-common, we could possibly clarify the specification for our track by adding a exercises/scrabble-score/HINTS.md - this is a special file that gets appended to the README.

Multiplying a letter value with a modifying token introduces dependency between characters of the word. This inter-character dependency makes it hard to keep the old map+fold method, which, in all its glorious functional-ness, doesn't like state between its iterations. We switch to a for-loop, which mutates state kept in the 'score' and 'previous_char_value' variables. Idiomatic rust prefers for-loops over map() if the primary effect of the iteration is the side-effect, as is now the case. Unfortunately, the inter-loop dependency sacrifices (potential) iteration parallelism. An alternative approach that could conserve parallelism would be to replace any "X:double" with "XX" and "X:triple" with "XXX". However, that capability is beyond std's String::replace(), and introducing the (admittedly awesome) regex-crate just for this is probably overkill.. The previous HashMap dictionary() is no longer suitable, as the value of some tokens ('2' and '3') now dynamically depends on the previous char. Also, Rust's match() is precisely 102% cooler than a HashMap, so use that!

juleskers · 2017-01-24T20:00:48Z

update!

I've #[ignored] all newly added tests
example code updated to pass all tests (locally ran check_exercises.sh, passed!)
slight tweak of one test name

feel free to nitpick over variable names and comments! It's the example after all, so it should be clear!

I'm opening a discussion issue on x-common next.

P.S. @petertseng thank you for your very polite and very welcoming responses! I appreciate it!
The details and helpful steps helped me greatly.
@IanWhitney thanks for pitching in!

petertseng

Given that x-common won't have it, now the Rust track should decide whether we want to add this, in contravention of x-common.

I honestly do not feel strongly either way, so if anyone does, you should speak up.

What I will say is that if we are to add it, we will want to specify how the double-letter scoring should be specified in exercises/scrabble-score/HINTS.md. I know Ian said:

In-line notation seem super awkward to me. Parsing seems fraught with problems, as you mention. I think a more robust structure is necessary.

Perhaps one would say that strings can only do so much for us. I suppose the best we can do with a string would be something like br[o:double]wn. Any alternatives?

petertseng · 2017-01-27T04:56:20Z

exercises/scrabble-score/example.rs

+    // reduce the multi-character value multipliers :double and :triple to a single char.
+    let tokenised_word = word.to_lowercase().replace(":double", "2").replace(":triple", "3");
+
+    for a_char in tokenised_word.chars() {


ah, I was about to say that char would be sufficient, but one probably can't use the name of a type as an identifier. makes sense to have a_char, then. Maybe c would be fine though, I'm OK with very short names for variables in use for very short time.

petertseng · 2017-01-27T04:56:55Z

exercises/scrabble-score/example.rs

-    values
+/// Calculates the value of the current character.
+///
+/// The previous characters' value is required in case the current


probably just characters' -> character's

petertseng · 2017-01-27T05:00:34Z

exercises/scrabble-score/example.rs

+///
+/// The previous characters' value is required in case the current
+/// token is a multiplier, which affects the score of the previous char.
+fn char_value(the_char: char, prev_value: u16) -> u16 {


I might also consider c here.

IanWhitney · 2017-01-28T14:06:13Z

A delimiter like [] works for me. Maybe multiplier-specific delimiters.

[] == double score
<> == triple score

aw[e]some
aw<e>some
[awesome]
<awesome>
<aw[e]some>
etc.

IanWhitney · 2017-01-28T14:08:52Z

And I'm fine having tests in place to handle the "extra credit" problems, even if those tests aren't in canonical.

juleskers · 2017-01-30T13:17:42Z

Ah, delimiters! I like how simple the implementation could become:
(I wonder if that compiles...? Mutating the multiplyer inside the match AND reading it in very the same expression looks maybe a bit too compact to work. But maybe Rust lets us have nice things!)

score += Match c {
   '[' => multiplyer *= 2; 0,
   ']' => multiplyer /= 2; 0,
   '<' => multiplyer *= 3; 0,
   '>' => multiplyer /= 3; 0,
   'a'|'e'|'u'.... => 1, 
   ... => 2,
   _ => 0 
} * multiplyer

Edit: Wow, it does work!

I also like how this would mean that there is no special-casing for word-multiplyer vs. letter-multiplyer.
The same parsing logic covers both!
Of course it may be desirable to do some paren-balancing sanity checks, but maybe not!

Edit 2: Seems my code-smell nose wasn't too far off: Clippy has doubts:

warning: unsequenced read of a variable,
note: whether read occurs before this write depends on evaluation order

Introducing an explicit temp-value solves this

IanWhitney · 2017-02-25T17:47:12Z

If I'm reading this PR correctly, there are just a few small changes @petertseng mentioned in comments. Are those blocking merge? Or can we move on this PR without those changes?

petertseng · 2017-02-25T21:44:44Z

I had a different reading of this PR: I thought all of my comments are unimportant, and what we would want to discuss before merge is:

In-line notation seem super awkward to me. Parsing seems fraught with problems, as you mention. I think a more robust structure is necessary.

And that whatever notation we use should be added to HINTS.md

IanWhitney · 2017-02-26T03:20:33Z

Ah, yes. I saw the comment approving the ideas of delimiters, but hadn't checked the code to see if delimiters had been implemented.

juleskers · 2017-02-26T09:51:12Z

After the discussion on the common track, I understood that the optional exercise was intentionally left vague. Figuring out a syntax was explicitly said to be part of the exercise.
That's why I haven't pursued this further.

IanWhitney · 2017-02-26T13:56:35Z

Ok. Sounds like we can close, then. Thanks for your work on this, @juleskers!

scrabble-score: add tests for the optional extensions :double and :tr…

eb40753

…iple

juleskers mentioned this pull request Jan 19, 2017

scrabble-score: add a test for the entire alphabet #250

Merged

petertseng reviewed Jan 21, 2017

View reviewed changes

Jules Kerssemakers added 3 commits January 24, 2017 18:26

#[ignore] newly added tests

f987bce

better test name for zero-scoring case

761ecfa

This was referenced Jan 24, 2017

scrabble-score: add tests for optional extensions? (:double/:triple letter values) exercism/problem-specifications#511

Closed

scrabble-score: clarify syntax for :double/:triple scoring letter/word? exercism/problem-specifications#512

Closed

petertseng reviewed Jan 27, 2017

View reviewed changes

IanWhitney closed this Feb 26, 2017

Tests for scrabble-score's optional extensions #249

Tests for scrabble-score's optional extensions #249

Uh oh!

Conversation

juleskers commented Jan 19, 2017

Uh oh!

juleskers commented Jan 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

petertseng left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IanWhitney commented Jan 21, 2017

Uh oh!

juleskers commented Jan 21, 2017

Uh oh!

petertseng commented Jan 21, 2017

Uh oh!

juleskers commented Jan 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

petertseng left a comment

Choose a reason for hiding this comment

Uh oh!

petertseng Jan 27, 2017

Choose a reason for hiding this comment

Uh oh!

petertseng Jan 27, 2017

Choose a reason for hiding this comment

Uh oh!

petertseng Jan 27, 2017

Choose a reason for hiding this comment

Uh oh!

IanWhitney commented Jan 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

IanWhitney commented Jan 28, 2017

Uh oh!

juleskers commented Jan 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

IanWhitney commented Feb 25, 2017

Uh oh!

petertseng commented Feb 25, 2017

Uh oh!

IanWhitney commented Feb 26, 2017

Uh oh!

juleskers commented Feb 26, 2017

Uh oh!

IanWhitney commented Feb 26, 2017

Uh oh!

Uh oh!

juleskers commented Jan 20, 2017 •

edited

Loading

petertseng left a comment •

edited

Loading

juleskers commented Jan 24, 2017 •

edited

Loading

IanWhitney commented Jan 28, 2017 •

edited

Loading

juleskers commented Jan 30, 2017 •

edited

Loading