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

Update to fix crypto text normalization. #28

Merged
merged 1 commit into from
Oct 15, 2014

Conversation

ClashTheBunny
Copy link
Contributor

This would update the readme for this exercise to be consistent with the source problem and to keep the idea of "square" more exact. If this is changed, then the following change should be rejected:
exercism/DEPRECATED.javascript#61

If this is rejected, that change should be accepted.

This would update the readme for this exercise to be consistent with the source problem and to keep the idea of "square" more exact.  If this is changed, then the following change should be rejected:
exercism/DEPRECATED.javascript#61

If this is rejected, that change should be accepted.
@ClashTheBunny
Copy link
Contributor Author

Chunks by 5:
Perl
Ruby
C++
Clojure
C#
Haskell
Go
Python doesn't explicitly test for this, but requires chunk by 5

JavaScript chunks like the original problem, fixed in exercism/DEPRECATED.javascript#61.

Either all the exercises and the read-me needs to be updated or exercism/DEPRECATED.javascript#61 should be implemented.

@kytrinyx
Copy link
Member

Thank you. I think this is the correct fix. I'll create a related issue for all the languages you listed as having the incorrect implementation.

It may be worth expanding on the square part of the crypto square, or creating a couple of extra examples, or even talking about how typically crypto text is broken into groups of five, but that the crypto square differs in this.

esaoh ghnss eoau
imtgdvs fearwer mayoogo
anouuio ntnnlvt wttddes
aohghn sseoau
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't look like the correct chunk size to me. The plaintext version is in chunks of 8, this is chunks of 7 except for the last two chunks which are 6. That's inconsistent with other behavior that I've seen.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you're right. The two spaces between the last two also look wrong, now that I give it a second look.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the cipher text is correct and that various current example solutions are incorrect. Compare with the the original problem description at the source referenced in the yml. The description in the readme looks consistent with the source document. This cipher text looks consistent with the descriptions and this is the cipher text given on the source page. I think it is the code which doesn't match the specification.

The two spaces between the last word is also copied from the source page. We could specify to put those extra spaces in, but I'd be in favor of taking them out of our readme. I think we should also show this on a single line,

imtgdvs fearwer mayoogo anouuio ntnnlvt wttddes aohghn sseoau

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I left a lengthy comment on the original ticket, #28. Sorry if it was the wrong place.

@ClashTheBunny ClashTheBunny deleted the patch-1 branch October 15, 2014 15:58
@ClashTheBunny
Copy link
Contributor Author

The two spaces were just to make things line up well. Sorry, too much clojure lately, 🍡.

I think the beauty of the algorithm as stated in the readme is that the maximum length difference between the long encrypted lengths and the short encrypted lengths of cipher text is 1. It's much more a "square" than a four groups of the same and one group of one.

It does take a bit of calculation to find the dimensions of the square again, as it is in the clojure example.clj patch that I sent in exercism/clojure#33. I think the tests having some good examples is important for people to actually grok what needs to happen.

One of the general issues that exist with this change is that it almost makes it harder the way people were "guided" to create their algorithm. In many of the languages' tests they are stepped through the development with "square size", "normalize plaintext", "cipher the text", then "format the text". I think the last two steps may be easier to combine and then people get the actual practice of transposing a matrix as well as not having to figure out the slightly more arcane chunking algorithm. I'm not sure of the pedagogical intent of the original, but as a person who works often with matrices, I would have liked to see something concerning that in the exercises. I haven't yet made it through one of the more developed courses, so I don't know if it's there, but I finished clojure and this was the closest thing to being able to practice that. It could even be an optional "advanced" addition to the readme like many exercises suggest. I could also work on converting a project euler problem or some other image processing problem to an exercise to add at the end.

The difference I see between the currently updated readme and the original is that the original doesn't specify that the crypto-square be square, just any rectangle.

@kytrinyx
Copy link
Member

I'm not sure of the pedagogical intent of the original

I'm not sure, either.

Here on exercism there is no pedagogical intent at all. All of the problems are just toy problems, anything that people learn from a practice session is accidental. At least, that's how it started.

As people started solving various problems, we started seeing how the problem could be tweaked and improved to highlight more interesting aspects of the problem.

as a person who works often with matrices, I would have liked to see something concerning that in the exercises.

Yeah, this would be interesting, and definitely in line with the "let's make this more interesting". I'm not sure whether it should be a separate, more advanced problem, or a tweak to this one.

the original doesn't specify that the crypto-square be square, just any rectangle.

I hadn't noticed that back when I wrote the original README. I like the square-only problem for aesthetic reasons, and would not be against keeping it that way here.

@soniakeys
Copy link
Contributor

I have one more observation about the original source problem. On closer reading, I don't think it is specifying to group or break the output at all. The examples it gives happen to have one less row than column which is the source of the spaces in the output. To illustrate, consider plain text "0123456789ABCD". The smallest square 14 characters fit in is a 4x4:

+ - - - - +
| 0 1 2 3 |
| 4 5 6 7 |
| 8 9 A B |
| C D     |
+ - - - - +

I think it would ask for the result 048C159D26A 37B. We could require this result but I think our current problem statement, "Output the encoded text grouped by column" leads to a more pleasing result. Either way, I think this artifact of the examples selected in the source problem is also a contributer to confusion on this problem..

@kytrinyx
Copy link
Member

I also like the current "grouped by column" problem statement, though I think you're right that the original problem can be read either way.

This was referenced Dec 4, 2014
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.

None yet

4 participants