Skip to content

simple-cipher: Add test to disallow mixed case key#219

Merged
masters3d merged 1 commit intoexercism:masterfrom
tekerson:patch-1
Sep 17, 2018
Merged

simple-cipher: Add test to disallow mixed case key#219
masters3d merged 1 commit intoexercism:masterfrom
tekerson:patch-1

Conversation

@tekerson
Copy link
Copy Markdown
Contributor

Hi,

During mentoring, I came across a solution that checks if the key is valid (i.e. only contains alpha ascii lower-case letters) by testing

if (key === key.toUpperCase()) {
    throw new Error('Bad key')
}

This passes the current "throws an error with an all caps key" test but allows mixed-case keys, which I believe should actually be invalid. So, I'm proposing a new test to test for this situation.

@tekerson
Copy link
Copy Markdown
Contributor Author

It looks like the example implementation fails this test 😮 (I actually didn't even know there was an example implementation… oops).

Is my assumption that the key should only contain lower case characters incorrect, or should I add a fix to the example implementation too?

@masters3d
Copy link
Copy Markdown
Contributor

Yes please.

A solution that checks if the key is valid (i.e. only contains alpha ascii lower-case letters) by testing that `if (key === key.toUpperCase()) { throw new Error('Bad key') }` passes the current "throws an error with an all caps key" test but allows mixed-case keys, which should actually be invalid.
@tekerson
Copy link
Copy Markdown
Contributor Author

I've updated the example implementation.
The regex was updated to pass the new test.
I did also slightly re-arranged the if..else block to avoid requiring the non-null assertion (!) operator.

But, it looks like CI broke for unrelated reasons;

We had an unexpected error preparing a VM for this build

I'm not sure if I can do anything about that, or if someone just needs to rerun the job?

@masters3d masters3d merged commit 5b3e540 into exercism:master Sep 17, 2018
@masters3d
Copy link
Copy Markdown
Contributor

Thanks!

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.

2 participants