add run-length-encoding #277
Conversation
divagant-martian
force-pushed
the
run-length-encoding
branch
from
893e467 to
bc87070
Compare
divagant-martian
changed the title
There was a problem hiding this comment.
Thanks for the work!
In addition to the comments, can you also add an empty file src/lib.rs as it was in #270?
| @@ -55,6 +55,15 @@ | |||
| ] | |||
| }, | |||
| { | |||
| "slug": "run-length-encoding", | |||
There was a problem hiding this comment.
I personally would have placed it later since unlike beer song we have to read an input string. My choice would have been somewhere after hamming but somewhere before wordy.
But that is just a personal feeling, so let me know if here is in fact appropriate
Comment is outdated - this used to be right after beer song. I am OK right right before hamming.
| version = "0.1.0" | ||
| authors = ["zombiefungus <[email protected]>"] | ||
|
|
||
| [dependencies] |
There was a problem hiding this comment.
just realised not all of our Cargo.toml have this line! Guess it's optional. The only exercise with a [dependencies] but no dependencies is acronym.
This line is included by cargo new though. So it makes sense to have.
| pub fn encode(input: &str) -> String { | ||
| input | ||
| .chars() | ||
| .fold((String::new(),' ',0,1), |(mut acc, last, last_n, pos), c| { |
There was a problem hiding this comment.
spaces after the commas in the tuple? To be consistent with the fact that there are spaces after commas in other places
| input | ||
| .chars() | ||
| .fold((String::new(), 0), |(mut acc, last_n), c| { | ||
| if c.is_digit(10) { // keep reading.. keeeep reading |
There was a problem hiding this comment.
is this comment meant to tell the reader of the code that it's necessary to keep reading (I don't see the value of this) or that the code should keep reading? Can the comment be clearer about what the code will read, in that case?
There was a problem hiding this comment.
haha sorry, the idea is that you can't conclude that the number of digits to place is c because there may be other digits latter. This is just a personal comment, i'll remove it
| .chars() | ||
| .fold((String::new(), 0), |(mut acc, last_n), c| { | ||
| if c.is_digit(10) { // keep reading.. keeeep reading | ||
| (acc, 10*last_n + c.to_digit(10).unwrap()) |
There was a problem hiding this comment.
Since there's an is_digit followed by a to_digit.unwrap, what about using if let above? Less need to trust that the unwrap will succeed.
There was a problem hiding this comment.
well, I am learning rust my self and I haven't used it. Maybe this should be implemented by someone with more experience
There was a problem hiding this comment.
Then my challenge is to try it.
| } | ||
|
|
||
| #[test] | ||
| // #[ignore] |
There was a problem hiding this comment.
whereas these should probably be uncommented, right?
I wanted to look for documentation where we say why we have tests ignored, but it is not present. Our documentation is not sufficient in this area. I will have to mention this in #279 .
| #[test] | ||
| // #[ignore] | ||
| fn test_decode_empty_string() { | ||
| assert_eq!("" , cipher::decode("")); |
There was a problem hiding this comment.
there's a space before the comma here, but not in most other cases. Should it be removed?
| #[test] | ||
| // #[ignore] | ||
| fn test_decode_string_with_no_single_characters() { | ||
| assert_eq!("AABBBCCCC" , cipher::decode("2A3B4C")); |
There was a problem hiding this comment.
there's a space before the comma here, but not in most other cases. Should it be removed?
| #[test] | ||
| // #[ignore] | ||
| fn test_decode_single_characters_with_repeated_characters() { | ||
| assert_eq!("WWWWWWWWWWWWBWWWWWWWWWWWWBBBWWWWWWWWWWWWWWWWWWWWWWWWB" , |
There was a problem hiding this comment.
there's a space before the comma here, but not in most other cases. Should it be removed?
| @@ -0,0 +1,87 @@ | |||
| extern crate run_length_encoding as cipher; | |||
There was a problem hiding this comment.
I'm unsure about it because I wouldn't consider run-length encoding as a means to obscure a message to prevent it from being read. That's what I think the definition of a cipher, but I apologise if I was mistaken since English isn't my first language. I'm not sure I have a better suggestion though - simply encoding, perhaps? Is shortening it to rle acceptable in Rust?
divagant-martian
force-pushed
the
run-length-encoding
branch
2 times, most recently
from
93e3f54 to
c9ccb53
Compare
divagant-martian
force-pushed
the
run-length-encoding
branch
from
c9ccb53 to
d80cdfb
Compare
|
nice, thanks |
|
There were no objections in 48 hours (and a few more). It is time to merge. Thank you for your contribution! |
No description provided.