Skip to content

simple cipher ignore spaces etc #851

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

Closed
Alexander-Taran opened this issue Aug 25, 2021 · 8 comments · Fixed by #852
Closed

simple cipher ignore spaces etc #851

Alexander-Taran opened this issue Aug 25, 2021 · 8 comments · Fixed by #852

Comments

@Alexander-Taran
Copy link

"only lowercase a-z are translated, rest are passed through"

And I've seen couple of solutions that simply emit non-letters and move on with key advanced as well.
The key for the test is "a" so the tests pass.

But it is unclear what was the original idea.
To advance the key but not encode non-letters or consume the key only while encoding.

How would the test look with a lengthier key?

@angelikatyborska
Copy link
Member

Hi @Alexander-Taran, thank you for reporting the issue!

But it is unclear what was the original idea.

The idea for the Exercise is that you shouldn't need to concern yourself with any kind of input validation or handling incorrect input. It's out of scope for this exercise.

The test case "only lowercase a-z are translated, rest are passed through" (https://github.com/exercism/elixir/blob/main/exercises/practice/simple-cipher/test/simple_cipher_test.exs#L75-L79) doesn't even exist in the problem specifications (https://github.com/exercism/problem-specifications/blob/main/exercises/simple-cipher/canonical-data.json) where the test definitions are shared between all tracks. The Elixir track came up with this test case on its own in 2017.

There have been multiple attempts to add input validation to this exercise to problem specifications but they all seem to have been rejected, e.g. exercism/problem-specifications#1298

Problem specifications is the repository where all Exercism tracks share data about exercises.

Since this test case is not related to Elixir's internals as a programming language, my desired approach to this problem would be to just remove it so that we don't even lead people thinking about invalid input, as per the organization-wide goal of the exercise. @jiegillet @neenjaw what do you think?

@Alexander-Taran
Copy link
Author

@angelikatyborska I see.
My concern is not about validating input.
Maybe it is this specific test which is not carefully crafted.

consider this inputs for encode function:

encode("aaaa","a")  ==  "aaaa"
encode("aaaa","ab")  ==  "abab"
encode("aa aa","a")  ==  "aa aa"
encode("aa aa","ab") ==  "ab ba" or  encode("aa aa","ab") == "ab ab" ?

It is not clear from anywhere in the exercise how not encoding spaces should be treated.

@jiegillet
Copy link
Contributor

Another way is to add a test:

  # This one exists already
  @tag :pending
  test "only lowercase a-z are translated, rest are passed through" do
    assert SimpleCipher.encode("this is a test!", "d") == "wklv lv d whvw!"
    assert SimpleCipher.decode("wklv lv d whvw!", "d") == "this is a test!"
  end

  @tag :pending
  test "passed through letters consume key letters" do
    assert SimpleCipher.encode("this is a test!", "abc") == "tiks ks c ugsu!"
    assert SimpleCipher.decode("tiks ks c ugsu!", "abc") == "this is a test!"
  end

I personally think this is a bit more satisfying for people who like to think outside the box and about real world application, as some programmers tend to do. Happy to do a quick PR.

@angelikatyborska
Copy link
Member

It is not clear from anywhere in the exercise how not encoding spaces should be treated.

I know, I understood the problem. My point about not validating input is that it doesn't matter how encoding spaces should be treated because spaces are not valid input. We shouldn't even be testing invalid input in this exercise.

However, I am happy to accept Jie's proposal that spaces do consume a part of the key if that will be more satisfying to people. But if I ever get somebody complaining about the exercise that they believe it should be the opposite, I will remove both tests 😂.

@jiegillet
Copy link
Contributor

But if I ever get somebody complaining about the exercise that they believe it should be the opposite, I will remove both tests 😂.

Agreed 🤝

@Alexander-Taran
Copy link
Author

Alexander-Taran commented Aug 26, 2021

@angelikatyborska remove both. (-:
After solving it with zipping enums I got a failing test with spaces.. and spent an evening figuring out how to not consume key material.. ((((-:
That's how I read it.
Do not encode == do not consume the key.

@jiegillet
Copy link
Contributor

🤣 I didn't think the complain would come so quickly 🤣

I must abide to my agreement then, let's remove both tests! I'll update my PR.

@angelikatyborska
Copy link
Member

@Alexander-Taran Done! When v3 launches on the first of September, this exercise will be up-to-date without the test for spaces.

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 a pull request may close this issue.

3 participants