Skip to content

pangram: Add test case with repeated letters in both upper and lower cases #10

New issue

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

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

Already on GitHub? # to your account

Merged
merged 1 commit into from
Jan 17, 2017
Merged

Conversation

stkent
Copy link
Contributor

@stkent stkent commented Jun 8, 2016

My initial solution to the pangram problem was as follows:

fun isPangram(sentence: String): Boolean {
    return sentence
            .filter { it.isLetter() }
            .toList()
            .distinct()
            .count() >= 26
}

This passes all the existing tests, but fails the newly-added test because I did not explicitly ignore case before calling .distinct.

@sdavids13
Copy link
Contributor

I believe it's legitimate to have the upper case also count as lowercase characters. Maybe your implementation would merely need:

fun isPangram(sentence: String): Boolean {
    return sentence.toLowerCase()
            .filter { it.isLetter() }
            .toList()
            .distinct()
            .count() >= 26
}

These tests were taken from the canonical test suite: https://github.com/exercism/x-common/blob/master/pangram.json so we would need to see if this should be pushed out to the rest of the languages.

CC @kytrinyx

@stkent
Copy link
Contributor Author

stkent commented Jun 8, 2016

Oh yes, my implementation was definitely faulty according to the problem statement even though it passed all the existing tests, and your fix is the difference between passing and failing the new test. Adding the new test to the default suite covers off the possibility that others may fall into a similar trap in which an implementation appears to be valid in the general case but isn't.

@stkent
Copy link
Contributor Author

stkent commented Jun 8, 2016

Related: is there any way to know which language test suites were generated using the canonical suite, and which were written manually? Thanks!

@sdavids13
Copy link
Contributor

I believe the ruby track has their tests generated from files, I'm not sure about all of the others. It would be nice to build a test generator for the Kotlin track, but it does get a little tricky.

@sdavids13
Copy link
Contributor

OHHH, I misread the test! I thought you were just adding FOX to the end of the statement, but in fact you replaced dog with FOX. Maybe we should update the test name to say: mixedCaseDuplicatedCharactersMissingDog, or perhaps would we get the same point across by updated the test to: assertTrue(Pangrams.isPangram("the quick brown fox jumps over the lazy dog FOX"))

@stkent
Copy link
Contributor Author

stkent commented Jun 8, 2016

On inspection I think that

assertTrue(Pangrams.isPangram("the quick brown fox jumps over the lazy dog FOX"))

would still pass with my original faulty implementation (I used inequality there as I was not sure how the German alphabet characters would be handled). The key is to disallow upper and lower case versions of the same character from being counted separately. I'm happy to rename to your suggestion (mixedCaseDuplicatedCharactersMissingDog) and push up again if that works for you?

@kytrinyx
Copy link
Member

I agree that if it's possible to submit an "obviously incorrect" solution, then our tests are not good enough, and that should be addressed in x-common (which I see that @sdavids13 is fixing already. Excellent).

@behrtam
Copy link

behrtam commented Nov 6, 2016

Started a second try with exercism/problem-specifications#439 (after @sdavids13 failed) to get this added.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants