crypto-square: Rewrite it to match x-common or to be more idiomatic.

[rbasso]

The way it is now, crypto-square has some problems:

• The functions' names are not very descriptive and do not match what is is x-common.
• The last - commented out - test expects a string representing a matrix, with spaces used for separating rows and representing missing elements. This was specified in x-common/crypto-square.json, but seems so badong...

At least, we need to review the functions' names.

Related to #194 and exercism/problem-specifications#356.

Important: This exercise was changed in branch hspec-fail-fast by #249 . Please use that branch for any PR related to it or wait until it's merged in master.

[rbasso]

The JSON file

The reference canonical-data.json asks for 4 functions.

Ignoring the last test - that we'll discuss later - they are related like this:

normalizedPlaintext :: String -> String

plaintextSegments :: String -> [String]
plaintextSegments = toSquare . normalizedPlaintext

encoded :: String -> String
encoded = concat . transpose . plaintextSegments

ciphertext :: String -> String
ciphertext = unwords . transpose . plaintextSegments

Why not?

• Each function is defined as a transformation over a previous function. A better design would expose the building blocks, not their combinations.
• Because of the above, the functions' names are not very descriptive.
• encoded is not needed for ciphertext.

En example solution to the exercise

The above design seems unidiomatic in a language where function composition is so easy. A more idiomatic solution would be:

normalize :: String -> String

toSquare :: [a] -> [[a]]

encode :: String -> String
encode = unwords . transpose . toSquare . normalize

Padding

The last test checks the padding of the "words" outputted by ciphertext.

This is not mentioned in description.md and also makes no sense IMHO:

• If the output represents a rectangle, it could be a list of unpadded Strings.
• If if represents an encoded text separated in blocks by spaces, a single space is enough.
• The spaces in the output String should not be at the same time padding and separators.

What do we have now?

In the Haskell track, currently we ask for the functions:

normalizePlaintext :: String -> String -- Just missing a 'd' in the name.

plaintextSegments :: String -> [String] -- Matches the JSON file.
plaintextSegments = toSquare . normalizePlaintext

ciphertext :: String -> String -- Matches `encoded` from the JSON file.
ciphertext = concat . transpose . plaintextSegments

normalizeCiphertext :: String -> String -- Matches `ciphertext` from them JSON file, without testing for padding.
normalizeCiphertext = unwords . transpose . plaintextSegments

What should we do?

[petertseng]

Ruby also wishes changes to their crypto-square exercism/ruby#422

Either way, should we fix the problem at the source by changing canonical-data.json?

Especially if the intermediate functions are removed, we will have a lot more freedom, since we can ask for less.

[rbasso]

Reading exercism/problem-specifications#250, it seems that keeping the intermediary functions was not an unpopular decision at the time. so I'm not sure if people will agree with removing them.

Anyway, we are not alone in the feeling that they are undesirable but, if we remove the intermediary tests, only three test will remain, and one of them has space-padded words. We need more comprehensive tests.

Ignoring the JSON file or a while, we could imagine a completely new test suite...

How could it be?

Given a list of input-output pairs for the encoding function, we could progressively test the following:

• Is the set of output characters correct?
• Is the multiset of output characters correct?
• The encoded message has the all the non-space characters in the correct order?
• Is the output message correctly separated in blocks?
• Are the blocks separated by a single space?
• Is the output string exactly as expected?

I think that the last two test should could be avoided.

This plan would add a lot o complexity to the tests, but it would keep a progression similar to the one using intermediary functions.

Is it too crazy?

[petertseng]

It sounds like the tests you describe will be test for particular properties of the outputs, rather than specific values! It's interesting because exercism/problem-specifications#191 was intended to be that sort of exercise as well. I'd be interested to see one.

[rbasso]

The "design" came out this way for two reasons:

• To avoid testing for intermediary functions.
• To avoid testing for equality on the output.

Instead of over-specifying the tests, the idea was to test progressively until it have the minimum desired behavior.