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

Add crypto-square exercise #75

Merged
merged 4 commits into from
Mar 10, 2018
Merged

Add crypto-square exercise #75

merged 4 commits into from
Mar 10, 2018

Conversation

jonmcalder
Copy link
Member

Implement tests and example solution
Generate README
Update config

Implement tests and example solution
Generate README
Update config
@jonmcalder
Copy link
Member Author

jonmcalder commented Aug 3, 2017

Following the advice of this discussion in exercism/problem-specifications, I opted to include intermediate methods in the tests here. Having said that, I think there might be quite a wide variety of ways to move from the input text to the ciphertext and these intermediate methods could get in the way of an elegant implementation rather than aiding in the learning process.

My example solution here cleanses the input string and then maps the characters out into a matrix which makes the 'encoding' process easy. This makes the intermediate methods plaintext_segments and encoded rather pointless though. Obviously an example solution is just an example, but I'm wondering whether maybe we shouldn't drop these for those two intermediate methods and just keep the first and last methods?

@stale stale bot added the wontfix label Oct 2, 2017
@exercism exercism deleted a comment from stale bot Oct 3, 2017
@stale stale bot removed the wontfix label Oct 3, 2017
@katrinleinweber
Copy link
Contributor

Sorry! I overlooked this the last 2 months, and won't have spare time in the next few weeks either.

@jonmcalder
Copy link
Member Author

Ok no worries. I don't think the approach of the example solution matters too much, but I'd like a 2nd opinion on whether these intermediate methods should be included in the starting snippet (and more importantly whether they should be tested), so will just keep this PR open for now until you can revisit it later.

@stale stale bot added the wontfix label Dec 2, 2017
@exercism exercism deleted a comment from stale bot Dec 2, 2017
@stale stale bot removed the wontfix label Dec 2, 2017
@stale stale bot added the wontfix label Jan 31, 2018
@exercism exercism deleted a comment from stale bot Feb 3, 2018
@stale stale bot removed the wontfix label Feb 3, 2018
Copy link
Contributor

@katrinleinweber katrinleinweber left a comment

Choose a reason for hiding this comment

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

I got all but the last test to go green. It's tricky, but fine to include.

@jonmcalder
Copy link
Member Author

Thanks! So you think it's fine to include the tests for the intermediate methods?

@katrinleinweber
Copy link
Contributor

katrinleinweber commented Feb 11, 2018

[…] maybe we shouldn't drop these for those two intermediate methods and just keep the first and last methods?

I think those did prevent me from finding a solution to the last test. I had duplicated strsplit() and paste() code into ciphertext that basically did the opposite of what that code did in the intermediate methods/functions. And trying to integrate the exceptions for rectangular matrices got out of hand :-/ OTOH I can't spend a few hours again to work out a better example.

Because I think it's better to have a really tricky puzzle in the track, than not, I'm OK with merging this. Please squash, because the two merges & conflict resolutions were this messy, because I tried the web-editor for a change, not a JSON-validating IDE :-/

@katrinleinweber
Copy link
Contributor

Hi @jonmcalder! Do you have any doubts about merging & squashing this?

@jonmcalder jonmcalder merged commit 4927956 into master Mar 10, 2018
@jonmcalder
Copy link
Member Author

Thanks @katrinleinweber! I must have missed/overlooked this somehow - I'm ok with it since you are. I'm sure we (or others) can refine it at a later stage if need be :-)

@jonmcalder jonmcalder deleted the add-crypto-square branch March 10, 2018 10:42
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