Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add "alphabet works" test to scrabble-score #509

Closed
juleskers opened this issue Jan 24, 2017 · 8 comments
Closed

add "alphabet works" test to scrabble-score #509

juleskers opened this issue Jan 24, 2017 · 8 comments

Comments

@juleskers
Copy link
Contributor

juleskers commented Jan 24, 2017

So, I'm working my way through the rust track, and have a minor nitpick with the scrabble exercise.
After passing all tests, the program still doesn't score all letters of the (English) alphabet. A few letters are not tested by any testcase.

I submitted a pull-request to add an alphabet test over at the rust track, but the kind maintainers of the rust-track where of the opinion this should be discussed "upstream", i.e. here, first, since:
a) this would be useful for all languages
b) the alphabet is not a valid scrabble word, and such cases were specifically excluded/removed historically. Reinserting a non-word testcase thus may not be desirable.

So:
question 1) should we have any test that verifies the alphabet
question 2) would it be OK to have a test that checks "score("abcdefghijklmnopqrstuvwxyz") == 87"

@robkeim
Copy link
Contributor

robkeim commented Jan 24, 2017

+1 for having a test that covers all of the letters, that makes sense to me

You could either accomplish that with the test you're suggesting or have a test that scores 26 times once for each letter of the alphabet.

@Insti
Copy link
Contributor

Insti commented Jan 25, 2017

  1. Yes.
  2. Yes.

The advice you have gotten already in that thread is good, the Rust track maintainers who replied to you are also active here.

@juleskers
Copy link
Contributor Author

OK! Thanks for the feedback!
You can expect a pull-request for data-common.json soon-ish.

@petertseng
Copy link
Member

It seems better for completeness of the tests that all the letters should be tested.

The old reason for doing non-alphabet words was #86. If we have changed our minds since then and/or think the alphabet is nevertheless a good idea, let's get it added. I have no argument to offer for either side on this.

@ErikSchierboom
Copy link
Member

Testing for all letters seems like a sensible test to add.

@IanWhitney
Copy link
Contributor

I think it depends on what we want students to learn here. Even with a partial list of letters the 'scoring' algorithm is done. And that algorithm is the interesting part, I think. Adding more letters is just configuration and I don't think it adds anything to the learning.

@juleskers
Copy link
Contributor Author

juleskers commented Jan 26, 2017

The algorithm is definitely the hard part, I agree.
As for learning goals: there is something to be said for teaching completeness, and that you always overlook something.
The discipline and effort to 'wrap up' and do the 'boring parts' is where I spend most of my time at $DAYJOB.

Example here: I only noticed I was missing letters when I updated my docstring to match the README, and noticed that an entire arm (8 pts) was missing from my implementation. I implicitly assumed, after so many words ("oxyphenbutazone" even!) I'd have everything.
That surprised/shocked me enough that I started this whole PR/Issue.

Additionally, Test-driven development would dictate that you never add code without a failing testcase first; we shouldn't encourage people to start on that slippery slope for such a trivial exercise.

@IanWhitney
Copy link
Contributor

If we can have the test expose missing logic/edge cases, like the ones you found, then I'm all for it.

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

No branches or pull requests

6 participants