Add rail-fence-cipher exercise #665
Conversation
We already have a fair number of ciphering exercises, but a few more never hurt. In particular, this one is interesting because it's all about text permutation, and it's not a block cipher: you can't generate any subset of the message without processing the entire thing at once. This means that it's unsuitable for standard text stream traits such as io::Read and io::Write, but by the same token, it means we can design a very simple API for the student to implement. This PR includes one test not present in the canonical data. The intent of this test is to ensure that students are handling individual characters, not just bytes. While the general policy for exercism is to avoid unicode unless it adds something to the exercise, I believe that in this case it adds something to the exercise: as a text permutation exercise, it makes sense to ensure that wide characters are handled properly. One potential extension not included in this exercise would be to add a case oriented around grapheme clusters. As grapheme clustering is fairly complicated and best accomplished through the use of external crates, such a thing would best be hidden behind a feature gate.
There was a problem hiding this comment.
In my review, I decided:
- The API presented to students is reasonable
- The tests provided to students are reasonable
- The example solution is not an egregiously convoluted way of solving the problem (notice the slightly lower level of rigour here, but it is because of the role the example solution plays versus the other components of the exercise)
Consider whether to change any optional comments noted here. Since they are so minor I am going to assume you do not wait for another review from me, instead trusting Travis CI. (I do not object if you wait for a review from reviewers other than me, if you choose)
|
|
||
| fn uncons(s: &str) -> (&str, &str) { | ||
| s.split_at(match s.chars().next() { | ||
| Some(c) => c.len_utf8(), |
There was a problem hiding this comment.
optional: it seems it would be possible to use https://doc.rust-lang.org/std/option/enum.Option.html#method.map_or here, if it doesn't sacrifice too much readability (in the service of... brevity, I guess?)
| RailFence(rails) | ||
| } | ||
|
|
||
| fn next(&self, down: &mut bool, rail: &mut usize) { |
There was a problem hiding this comment.
Optional:
I see, yes, because this function both depends on the current values of down and rail but may also change them.
If someone demands the removal of &mut, I would then suggest that this function could return the tuple (should_down_be_inverted, delta_rail), and let the caller adjust down and rail accordingly.
However, that doesn't have much of a benefit, not even to indicate at the call site how down and rail might change in response to next. The caller still has to read next to understand the possible values of the returned tuple.
So I see no benefit to removing &mut here.
There was a problem hiding this comment.
Another factor in favor of writing it in this way: destructuring assignment is still not allowed outside a let binding, making it even less convenient to update values by returning a function.
| /// in terms of this function. | ||
| fn process_encode_case(input: &str, rails: u32, expected: &str) { | ||
| let rail_fence = RailFence::new(rails); | ||
| assert_eq!(rail_fence.encode(input), expected,); |
There was a problem hiding this comment.
the trailing comma seems unusual for situations where the closing parenthesis is on the same line
| /// in terms of this function. | ||
| fn process_decode_case(input: &str, rails: u32, expected: &str) { | ||
| let rail_fence = RailFence::new(rails); | ||
| assert_eq!(rail_fence.decode(input), expected,); |
There was a problem hiding this comment.
the trailing comma seems unusual for situations where the closing parenthesis is on the same line
| "difficulty": 4, | ||
| "topics": [ | ||
| "chars", | ||
| "cipher", |
There was a problem hiding this comment.
perhaps it would be useful to have a separate PR add that topic to the other cipher exercises.
That is only important if there's a filter-by-topic functionality on the website, and I'm not yet sure there is such a one.
| pub struct RailFence; | ||
|
|
||
| impl RailFence { | ||
| pub fn new(rails: u32) -> RailFence { |
There was a problem hiding this comment.
I think I understand why the API is this way. It subtly encourages computing any calculations specific to the number of rails only once. That seems reasonable.
Behaviour might be unpredictable if someone passes 0, but we are not testing the behaviour so I don't see a need for a change.
We already have a fair number of ciphering exercises, but a few
more never hurt.
In particular, this one is interesting because it's all about text
permutation, and it's not a block cipher: you can't generate any
subset of the message without processing the entire thing at once.
This means that it's unsuitable for standard text stream traits
such as
io::Readandio::Write, but by the same token, it means wecan design a very simple API for the student to implement.
This PR includes one test not present in the canonical data. The
intent of this test is to ensure that students are handling individual
characters, not just bytes. While the general policy for exercism
is to avoid unicode unless it adds something to the exercise, I believe
that in this case it adds something to the exercise: as a text permutation
exercise, it makes sense to ensure that wide characters are handled properly.
One potential extension not included in this exercise would be to add
a case oriented around grapheme clusters. As grapheme clustering is
fairly complicated and best accomplished through the use of external
crates, such a thing would best be hidden behind a feature gate.