simple-cipher: ambiguous instructions and conflicting files

sbromberger commented 3 years ago

simple-cipher bundles two .go files - cipher.go which defines an interface and simple_cipher.go which holds a package definition. It is unclear what the purpose of cipher.go is - in fact, I didn't even see it there when I started the exercise.

Following on, there is no indication outside of cipher.go that the exercise is intended to teach/demonstrate interfaces. This should be clear in the instructions, or the content of cipher.go should be moved into simple_cipher.go so that people opening the exercise will see what's intended.

The bigger picture would include tags on exercises that showed the concepts to be practiced, but that's probably worth a separate discussion.

sbromberger commented 3 years ago

Also - the instructions don't suggest that a shift cipher take an integer as a key - the instructions show a string of repeated characters.

There is also no discussion of Vigenere ciphers in the instructions, but they're necessary for the tests.

The instructions also mention

Step 3

The weakest link in any cipher is the human being. Let's make your substitution cipher a little more fault tolerant by providing a source of randomness and ensuring that the key contains only lowercase letters.

If someone doesn't submit a key at all, generate a truly random key of at least 100 lowercase characters in length.

But there are no tests for this (the obvious Go way of doing this by submitting an empty string as a key fails the test.)

sbromberger commented 3 years ago

Finally, there is no documentation on what constitutes an invalid Vigenere key. As far as I can tell, it's len < 3 or any chars not in a-z.

junedev commented 3 years ago

Thanks for opening the issue. Here some answers to your questions.

A Why does the exercise use an interface?

It is unclear what the purpose of cipher.go is

Imagine what the API would look like without the interface. Maybe something like this:

ShiftCipherEncode(distance int, text string) string
ShiftCipherDecode(distance int, text string) string

And this would then be repeated for the other two ciphers. That work of course but I think abstracting the common actions into the interface makes sense. It helps with the testing in our case and in a real world scenario it would help to make the exact cipher method easily replaceable if someone decides to use something better.

B Quality of instructions

in fact, I didn't even see [cipher.go] there when I started the exercise.

Also - the instructions don't suggest that a shift cipher take an integer as a key - the instructions show a string of repeated characters. There is also no discussion of Vigenere ciphers in the instructions, but they're necessary for the tests.

Finally, there is no documentation on what constitutes an invalid Vigenere key.

The instructions clearly refer to cipher.go and explain the interface (see screenshot below). They also show the signatures for all the ciphers and explain valid keys. However, due to a formatting error, this part of the instructions is wrongly grouped under "Extensions". Maybe you missed it because of that? (I also noticed when previewing the instructions before starting the exercise, this part is not shown.)

there is no indication outside of cipher.go that the exercise is intended to teach/demonstrate interfaces. This should be clear in the instructions

Please keep in mind that simple cipher is not a learning exercise, it is a practice exercise. Practice exercises do not need to teach/demonstrate a specific concept, they can theoretically require you to know any number of things to solve them in a good way and they can usually be solved in multiple ways. As already mentioned to you on Slack, we will add "prerequisites" and "practices" keys for practice exercises in the future (https://github.com/exercism/go/issues/1396) but that is rather hard work and will quite some time. Once we get there, simple cipher would then only unlock after someone completed the interfaces concept exercise.

C Why is there a separate file cipher.go?

the content of cipher.go should be moved into simple_cipher.go so that people opening the exercise will see what's intended

The content of cipher.go is also used in the tests, not just as reference for the actual implementation so it is a bit "shared". If it would be part of simple_cipher.go a website student could accidentally mess up that bit of code and never be able to run the tests properly anymore. By putting the interface definition in a separate file, this file can be made read-only in the browser to avoid this kind of problem. The interface definition could potentially also be moved to the test file but that does not change much compared to the current version.

Tasks

I see two main tasks here to address the issue.

Fix the formatting issue that instructions.append.md is rendered as part of "Extensions".
Fix the inconsistency regarding the key generation if no key (an empty string) was supplied. ("If someone doesn't submit a key at all, generate a truly random key of at least 100 lowercase characters in length.")

@sbromberger Judging from what you read above, would that be enough in your opinion?

sbromberger commented 3 years ago

Maybe the issue is that I assumed that the instructions on the exercise page were the same as in the README. Here's what I saw when I started:

As far as the API goes, I started by doing something like...

type Shift = int

func NewShift(x int) (Shift, error) {
    // error checks happen here
    return Shift(x), nil

func NewCaesar() (Shift, error) {
    return Shift(3)
}

func (c Shift) Encode(plaintext string) string { ... }
func (c Shift) Decode(ciphertext string) string { ... }

because there was no mention of Vigenère anywhere that I could see, this made sense since Caesar is just a specific case of a shift cipher.

Please keep in mind that simple cipher is not a learning exercise, it is a practice exercise. Practice exercises do not need to teach/demonstrate a specific concept, they can theoretically require you to know any number of things to solve them in a good way and they can usually be solved in multiple ways.

I get this, but the problem is that the tests are so prescriptive that there's really only ONE way to solve this. That is, my code fails because the tests want to compare the return of NewShift to nil, which doesn't make sense in this particular implementation.

In any case - is it possible to make sure that the instructions in the README are on the exercise page itself?

junedev commented 3 years ago

I am in the process of clarifying whether it is a bug or a feature that the append part is missing on the exercise overview page.

As for forcing some type of implementation: this is a consequence of Exercism being a platform that provides pre-written tests for the exercises. The tests will always force some kind of high level API. (Remember that in TDD one reason you write the tests first is to define this API.) The API might feel more rigid for strongly typed languages like Go but the constraint always exists and there is nothing we can do about it.

So the exercises are actually never about defining this high level API yourself, the task is to fill in the actual implementation code (the function bodies if you will). In the old days students always had to "guess" the API from looking at the test cases. Nowadays we say the API should be either described in the instructions or provided in the stub file (where you write your code later). In Go there are a lot of exercises that do neither of those and we are currently working on fixing this. https://github.com/exercism/go/issues/1893

junedev commented 3 years ago

@sbromberger The missing part on the exercise overview page is a bug: https://github.com/exercism/exercism/issues/6063 Erik will try to look into it this week.

abatkin commented 2 years ago

I'll just point out that whatever it is that gets displayed on the website (versus in the README.md file which I just realized contains different - and actually useful - information) is simply wrong in that if you attempt to try to solve the problem using just what is on the website (and trying to deconstruct the various tests) it is almost impossible to complete the exercise. For example, if you try to complete "Step 3" you will fail the tests (an empty key should (apparently) return a nil string, not generate a random key).

Honestly, it would be better if (until the website is fixed?) all of the website text was replaced with "Go read the README.md file".

junedev commented 2 years ago

@abatkin The instructions on the website and the README file are constructed using the same files. As explained before, there is a bug on the website, that a part that can contain vital information to solve the exercise is not shown on the exercise overview page. If you click "start/continue in editor" all instructions are shown correctly. The fix for the exercise overview page on the website was already implemented and will go live soon. Additionally, as said above, we are working on providing proper stub functions/methods for each exercise. (This is tracked in #1893.)

Regarding the issue with the empty key and generating a random key, I will create a separate issue for that so that someone working on this is not confused by all other things discussed here.

The issue with the missing header for the appended section was fixed already (https://github.com/exercism/go/pull/1897).

I will close this issue. If someone feels there is something open to be addressed that wasn't covered yet, leave a note and I will re-open.

4rc0s commented 6 months ago

It seems this is still an issue? If you follow the instructions ("If someone doesn't submit a key at all, generate a truly random key of at least 100 lowercase characters in length.") your code will fail the test ("Argument for NewVigenere must consist of lower case letters a-z only. Values consisting entirely of the letter 'a' are disallowed. For invalid arguments NewVigenere returns nil.") I believe this is the test:

func TestVigenereWrongKey(t *testing.T) {
    for _, k := range []string{"", "a", "aa", "no way", "CAT", "3", "and,"} {
        if NewVigenere(k) != nil {
            t.Errorf("NewVigenere(%q): got non-nil, want nil", k)
        }
    }
}

andrerfcsantos commented 3 months ago

@4rc0s If you think there's still an issue with this, please open a new forum post in the Go section: https://forum.exercism.org/c/programming/go/19

In your post, please include an example of code that you think should pass the tests, but fails that particular one.

exercism / go