Alphabet cypher in Rust
Clash Royale CLAN TAG#URR8PPP
.everyoneloves__top-leaderboard:empty,.everyoneloves__mid-leaderboard:empty margin-bottom:0;
up vote
5
down vote
favorite
This is my attempt at an alphabet cypher in Rust. I'm unsure if iteration and pushing to a mutable is the best way to handle building the output; it seems like a good candidate for a map but I can't figure out how to do it. Can anyone help?
use std::env;
fn main()
let keyword = env::args().nth(1).unwrap();
let body = env::args().nth(2).unwrap();
let transpose_table = get_transpose_table(keyword);
let mut output = String::new();
for (input_idx, input_char) in body.char_indices()
let transpose_distance = transpose_table[input_idx % transpose_table.len()];
let transposed_char_idx = char_to_alpha_idx(input_char) + transpose_distance;
output.push(alpha_idx_to_char(transposed_char_idx % 26));
println!("", output);
fn get_transpose_table(keyword: String) -> Vec<usize>
return keyword.chars()
.map(
fn char_to_alpha_idx(character: char) -> usize
return "abcdefghijklmnopqrstuvwxyz".find(character)
.expect("Bad char for char_to_alpha_idx");
fn alpha_idx_to_char(loc: usize) -> char
return "abcdefghijklmnopqrstuvwxyz".chars()
.nth(loc)
.expect("Bad loc for alpha_idx_to_char");
beginner rust caesar-cipher
add a comment |Â
up vote
5
down vote
favorite
This is my attempt at an alphabet cypher in Rust. I'm unsure if iteration and pushing to a mutable is the best way to handle building the output; it seems like a good candidate for a map but I can't figure out how to do it. Can anyone help?
use std::env;
fn main()
let keyword = env::args().nth(1).unwrap();
let body = env::args().nth(2).unwrap();
let transpose_table = get_transpose_table(keyword);
let mut output = String::new();
for (input_idx, input_char) in body.char_indices()
let transpose_distance = transpose_table[input_idx % transpose_table.len()];
let transposed_char_idx = char_to_alpha_idx(input_char) + transpose_distance;
output.push(alpha_idx_to_char(transposed_char_idx % 26));
println!("", output);
fn get_transpose_table(keyword: String) -> Vec<usize>
return keyword.chars()
.map(
fn char_to_alpha_idx(character: char) -> usize
return "abcdefghijklmnopqrstuvwxyz".find(character)
.expect("Bad char for char_to_alpha_idx");
fn alpha_idx_to_char(loc: usize) -> char
return "abcdefghijklmnopqrstuvwxyz".chars()
.nth(loc)
.expect("Bad loc for alpha_idx_to_char");
beginner rust caesar-cipher
add a comment |Â
up vote
5
down vote
favorite
up vote
5
down vote
favorite
This is my attempt at an alphabet cypher in Rust. I'm unsure if iteration and pushing to a mutable is the best way to handle building the output; it seems like a good candidate for a map but I can't figure out how to do it. Can anyone help?
use std::env;
fn main()
let keyword = env::args().nth(1).unwrap();
let body = env::args().nth(2).unwrap();
let transpose_table = get_transpose_table(keyword);
let mut output = String::new();
for (input_idx, input_char) in body.char_indices()
let transpose_distance = transpose_table[input_idx % transpose_table.len()];
let transposed_char_idx = char_to_alpha_idx(input_char) + transpose_distance;
output.push(alpha_idx_to_char(transposed_char_idx % 26));
println!("", output);
fn get_transpose_table(keyword: String) -> Vec<usize>
return keyword.chars()
.map(
fn char_to_alpha_idx(character: char) -> usize
return "abcdefghijklmnopqrstuvwxyz".find(character)
.expect("Bad char for char_to_alpha_idx");
fn alpha_idx_to_char(loc: usize) -> char
return "abcdefghijklmnopqrstuvwxyz".chars()
.nth(loc)
.expect("Bad loc for alpha_idx_to_char");
beginner rust caesar-cipher
This is my attempt at an alphabet cypher in Rust. I'm unsure if iteration and pushing to a mutable is the best way to handle building the output; it seems like a good candidate for a map but I can't figure out how to do it. Can anyone help?
use std::env;
fn main()
let keyword = env::args().nth(1).unwrap();
let body = env::args().nth(2).unwrap();
let transpose_table = get_transpose_table(keyword);
let mut output = String::new();
for (input_idx, input_char) in body.char_indices()
let transpose_distance = transpose_table[input_idx % transpose_table.len()];
let transposed_char_idx = char_to_alpha_idx(input_char) + transpose_distance;
output.push(alpha_idx_to_char(transposed_char_idx % 26));
println!("", output);
fn get_transpose_table(keyword: String) -> Vec<usize>
return keyword.chars()
.map(
fn char_to_alpha_idx(character: char) -> usize
return "abcdefghijklmnopqrstuvwxyz".find(character)
.expect("Bad char for char_to_alpha_idx");
fn alpha_idx_to_char(loc: usize) -> char
return "abcdefghijklmnopqrstuvwxyz".chars()
.nth(loc)
.expect("Bad loc for alpha_idx_to_char");
beginner rust caesar-cipher
edited Apr 13 at 6:46
asked Apr 12 at 14:57
Simon Brahan
1523
1523
add a comment |Â
add a comment |Â
1 Answer
1
active
oldest
votes
up vote
7
down vote
accepted
Disclaimer: I'm a Rust beginner with a background in C and C++. Now that I've lost my credibility, let's have a look at your code.
Use rustfmt, clippy and rustc
There are (at least) three helpful tools when one writes Rust code: rustfmt
, clippy
and the compiler rustc itself.
Rustfmt provides a common ground
The compiler doesn't yield any warnings, but rustfmt will change your code.
As many Rust developers use rustfmt
(or cargo fmt
, which calls the former), you should also make a habit of using it. That way others will feel right at home if they look at your code, and the same will hold for you when you look at theirs.
Clippy's warnings
clippy
on the other hand will catch common mistakes. In this case its mostly the redundant returns
in your idx
functions:
fn get_transpose_table(keyword: String) -> Vec<usize> char_to_alpha_idx(character))
.collect()
fn char_to_alpha_idx(character: char) -> usize
"abcdefghijklmnopqrstuvwxyz"
.find(character)
.expect("Bad char for char_to_alpha_idx")
fn alpha_idx_to_char(loc: usize) -> char
"abcdefghijklmnopqrstuvwxyz"
.chars()
.nth(loc)
.expect("Bad loc for alpha_idx_to_char")
There is no need to return
here. Next, you never consume the String
in get_transpose_table
, so we could borrow it instead with &String
. But in the end, we never use any String
-specific functionality, therefore we can just use &str
.
While we're at it, map(|x| func(x))
is just map(func)
, so let's change that as-well:
fn get_transpose_table(keyword: &str) -> Vec<usize>
keyword.chars().map(char_to_alpha_idx).collect()
All those small improvements were reported by clippy
.
Use T::with_capacity
if you know how many elements you will have
For your main algorithm, you should use String::with_capacity
if you know the number of characters you want to store:
let mut output = String::with_capacity(input.len());
Nothing else in your code changes.
Closures and readability
Apart from better names (message
instead of body
, body_index
or message_ index
instead of input_index
) that's all we can do with this algorithm. We could transform it into a closure:
let output: String = message
.char_indices()
.map(|(index, message_char)|
let transpose_distance = transpose_table[index % transpose_table.len()];
let transposed_char_idx = char_to_alpha_idx(message_char) + transpose_distance;
alpha_idx_to_char(transposed_char_idx % 26)
)
.collect();
but we only removed mutability. So let's get back to the drawing table. Why do we need index
? Because we want to know which usize
we should use from the transpose_table
. We would like to traverse a cycle of those values at the same time as our original message.
That's exactly what cycle()
and zip()
are for:
let output: String = transpose_table
.iter()
.cycle()
.zip(message.chars())
.map(|(transpose_distance, message_char)|
let transposed_char_idx = char_to_alpha_idx(message_char) + transpose_distance;
alpha_idx_to_char(transposed_char_idx % 26)
)
.collect();
Whether that's easier to understand than your original variant is left to you. However, we can also get rid of the intermediate Vec
:
let output: String = keyword
.chars()
.map(char_to_alpha_idx)
.cycle()
.zip(message.chars())
.map(|(transpose_distance, message_char)|
let transposed_char_idx = char_to_alpha_idx(message_char) + transpose_distance;
alpha_idx_to_char(transposed_char_idx % 26)
)
.collect();
This is a handful, but it works exactly as your previous variant.
Encapsulate functionality in functions
Now, my main critique point is that you don't provide a single function for your cipher. It's hard to test your functionality at the moment, so we better add one and some documentation:
/// Applies [The Alphabet Cipher] given by `keyword` to `message`.
///
/// Both `message` and `keyword` must only contain lowercase alphabetic ASCII
/// characters.
///
/// # Panics
/// Panics if any non-lowercase or non-alphabetic ASCII character is encountered.
///
/// # Examples
/// The character 'a' in the keyword won't change the input:
/// ```
/// assert_eq!(alphabet_cipher("aaaaa", "hello"), "hello");
/// ```
///
/// They keyword gets cycled if its shorter than the message:
/// ```
/// assert_eq!(alphabet_cipher("a", "hello"), "hello");
/// assert_eq!(alphabet_cipher("b", "hello"), "ifmmp");
/// assert_eq!(alphabet_cipher("ab","hello"), "hflmo");
/// assert_eq!(alphabet_cipher("ba","hello"), "iemlp");
/// ```
///
/// [The Alphabet Cipher]: https://en.wikipedia.org/wiki/The_Alphabet_Cipher
fn alphabet_cipher(keyword: &str, message: &str) -> String (offset, message_char)
The code snippets in the comments also get tested by cargo test
, but we can also add some more tests to check that our functions work, for example:
#[test]
fn alphabet_cipher_rot_a_to_z()
assert_eq!(
alphabet_cipher("abcdefghijklmnopqrstuvwxyz", "aaaaaaaaaaaaaaaaaaaaaaaaaa"),
"abcdefghijklmnopqrstuvwxyz"
);
There are more tests one can come up with, but those are left as an exercise.
User friendly interface
You use unwrap()
quite a lot. However, that leaves a user with unhelpful error messages:
$ ./cypher test
thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', ...
$ ./cypher test test12345
thread 'main' panicked at 'Bad char for char_to_alpha_idx', ...
Those error messages aren't that helpful. They're fine for toy programs, but you should view unwrap
as "I know that this will be Ok
or Some
, and it's fine to panic if it isn't, because I don't know how to continue". However, even in those cases, the end user should know how to continue. Keep that in mind when you write larger programs.
1
Wow. I'd love to see what you had to share if you weren't a beginner... Thanks for this.
â Simon Brahan
Apr 13 at 6:54
add a comment |Â
1 Answer
1
active
oldest
votes
1 Answer
1
active
oldest
votes
active
oldest
votes
active
oldest
votes
up vote
7
down vote
accepted
Disclaimer: I'm a Rust beginner with a background in C and C++. Now that I've lost my credibility, let's have a look at your code.
Use rustfmt, clippy and rustc
There are (at least) three helpful tools when one writes Rust code: rustfmt
, clippy
and the compiler rustc itself.
Rustfmt provides a common ground
The compiler doesn't yield any warnings, but rustfmt will change your code.
As many Rust developers use rustfmt
(or cargo fmt
, which calls the former), you should also make a habit of using it. That way others will feel right at home if they look at your code, and the same will hold for you when you look at theirs.
Clippy's warnings
clippy
on the other hand will catch common mistakes. In this case its mostly the redundant returns
in your idx
functions:
fn get_transpose_table(keyword: String) -> Vec<usize> char_to_alpha_idx(character))
.collect()
fn char_to_alpha_idx(character: char) -> usize
"abcdefghijklmnopqrstuvwxyz"
.find(character)
.expect("Bad char for char_to_alpha_idx")
fn alpha_idx_to_char(loc: usize) -> char
"abcdefghijklmnopqrstuvwxyz"
.chars()
.nth(loc)
.expect("Bad loc for alpha_idx_to_char")
There is no need to return
here. Next, you never consume the String
in get_transpose_table
, so we could borrow it instead with &String
. But in the end, we never use any String
-specific functionality, therefore we can just use &str
.
While we're at it, map(|x| func(x))
is just map(func)
, so let's change that as-well:
fn get_transpose_table(keyword: &str) -> Vec<usize>
keyword.chars().map(char_to_alpha_idx).collect()
All those small improvements were reported by clippy
.
Use T::with_capacity
if you know how many elements you will have
For your main algorithm, you should use String::with_capacity
if you know the number of characters you want to store:
let mut output = String::with_capacity(input.len());
Nothing else in your code changes.
Closures and readability
Apart from better names (message
instead of body
, body_index
or message_ index
instead of input_index
) that's all we can do with this algorithm. We could transform it into a closure:
let output: String = message
.char_indices()
.map(|(index, message_char)|
let transpose_distance = transpose_table[index % transpose_table.len()];
let transposed_char_idx = char_to_alpha_idx(message_char) + transpose_distance;
alpha_idx_to_char(transposed_char_idx % 26)
)
.collect();
but we only removed mutability. So let's get back to the drawing table. Why do we need index
? Because we want to know which usize
we should use from the transpose_table
. We would like to traverse a cycle of those values at the same time as our original message.
That's exactly what cycle()
and zip()
are for:
let output: String = transpose_table
.iter()
.cycle()
.zip(message.chars())
.map(|(transpose_distance, message_char)|
let transposed_char_idx = char_to_alpha_idx(message_char) + transpose_distance;
alpha_idx_to_char(transposed_char_idx % 26)
)
.collect();
Whether that's easier to understand than your original variant is left to you. However, we can also get rid of the intermediate Vec
:
let output: String = keyword
.chars()
.map(char_to_alpha_idx)
.cycle()
.zip(message.chars())
.map(|(transpose_distance, message_char)|
let transposed_char_idx = char_to_alpha_idx(message_char) + transpose_distance;
alpha_idx_to_char(transposed_char_idx % 26)
)
.collect();
This is a handful, but it works exactly as your previous variant.
Encapsulate functionality in functions
Now, my main critique point is that you don't provide a single function for your cipher. It's hard to test your functionality at the moment, so we better add one and some documentation:
/// Applies [The Alphabet Cipher] given by `keyword` to `message`.
///
/// Both `message` and `keyword` must only contain lowercase alphabetic ASCII
/// characters.
///
/// # Panics
/// Panics if any non-lowercase or non-alphabetic ASCII character is encountered.
///
/// # Examples
/// The character 'a' in the keyword won't change the input:
/// ```
/// assert_eq!(alphabet_cipher("aaaaa", "hello"), "hello");
/// ```
///
/// They keyword gets cycled if its shorter than the message:
/// ```
/// assert_eq!(alphabet_cipher("a", "hello"), "hello");
/// assert_eq!(alphabet_cipher("b", "hello"), "ifmmp");
/// assert_eq!(alphabet_cipher("ab","hello"), "hflmo");
/// assert_eq!(alphabet_cipher("ba","hello"), "iemlp");
/// ```
///
/// [The Alphabet Cipher]: https://en.wikipedia.org/wiki/The_Alphabet_Cipher
fn alphabet_cipher(keyword: &str, message: &str) -> String (offset, message_char)
The code snippets in the comments also get tested by cargo test
, but we can also add some more tests to check that our functions work, for example:
#[test]
fn alphabet_cipher_rot_a_to_z()
assert_eq!(
alphabet_cipher("abcdefghijklmnopqrstuvwxyz", "aaaaaaaaaaaaaaaaaaaaaaaaaa"),
"abcdefghijklmnopqrstuvwxyz"
);
There are more tests one can come up with, but those are left as an exercise.
User friendly interface
You use unwrap()
quite a lot. However, that leaves a user with unhelpful error messages:
$ ./cypher test
thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', ...
$ ./cypher test test12345
thread 'main' panicked at 'Bad char for char_to_alpha_idx', ...
Those error messages aren't that helpful. They're fine for toy programs, but you should view unwrap
as "I know that this will be Ok
or Some
, and it's fine to panic if it isn't, because I don't know how to continue". However, even in those cases, the end user should know how to continue. Keep that in mind when you write larger programs.
1
Wow. I'd love to see what you had to share if you weren't a beginner... Thanks for this.
â Simon Brahan
Apr 13 at 6:54
add a comment |Â
up vote
7
down vote
accepted
Disclaimer: I'm a Rust beginner with a background in C and C++. Now that I've lost my credibility, let's have a look at your code.
Use rustfmt, clippy and rustc
There are (at least) three helpful tools when one writes Rust code: rustfmt
, clippy
and the compiler rustc itself.
Rustfmt provides a common ground
The compiler doesn't yield any warnings, but rustfmt will change your code.
As many Rust developers use rustfmt
(or cargo fmt
, which calls the former), you should also make a habit of using it. That way others will feel right at home if they look at your code, and the same will hold for you when you look at theirs.
Clippy's warnings
clippy
on the other hand will catch common mistakes. In this case its mostly the redundant returns
in your idx
functions:
fn get_transpose_table(keyword: String) -> Vec<usize> char_to_alpha_idx(character))
.collect()
fn char_to_alpha_idx(character: char) -> usize
"abcdefghijklmnopqrstuvwxyz"
.find(character)
.expect("Bad char for char_to_alpha_idx")
fn alpha_idx_to_char(loc: usize) -> char
"abcdefghijklmnopqrstuvwxyz"
.chars()
.nth(loc)
.expect("Bad loc for alpha_idx_to_char")
There is no need to return
here. Next, you never consume the String
in get_transpose_table
, so we could borrow it instead with &String
. But in the end, we never use any String
-specific functionality, therefore we can just use &str
.
While we're at it, map(|x| func(x))
is just map(func)
, so let's change that as-well:
fn get_transpose_table(keyword: &str) -> Vec<usize>
keyword.chars().map(char_to_alpha_idx).collect()
All those small improvements were reported by clippy
.
Use T::with_capacity
if you know how many elements you will have
For your main algorithm, you should use String::with_capacity
if you know the number of characters you want to store:
let mut output = String::with_capacity(input.len());
Nothing else in your code changes.
Closures and readability
Apart from better names (message
instead of body
, body_index
or message_ index
instead of input_index
) that's all we can do with this algorithm. We could transform it into a closure:
let output: String = message
.char_indices()
.map(|(index, message_char)|
let transpose_distance = transpose_table[index % transpose_table.len()];
let transposed_char_idx = char_to_alpha_idx(message_char) + transpose_distance;
alpha_idx_to_char(transposed_char_idx % 26)
)
.collect();
but we only removed mutability. So let's get back to the drawing table. Why do we need index
? Because we want to know which usize
we should use from the transpose_table
. We would like to traverse a cycle of those values at the same time as our original message.
That's exactly what cycle()
and zip()
are for:
let output: String = transpose_table
.iter()
.cycle()
.zip(message.chars())
.map(|(transpose_distance, message_char)|
let transposed_char_idx = char_to_alpha_idx(message_char) + transpose_distance;
alpha_idx_to_char(transposed_char_idx % 26)
)
.collect();
Whether that's easier to understand than your original variant is left to you. However, we can also get rid of the intermediate Vec
:
let output: String = keyword
.chars()
.map(char_to_alpha_idx)
.cycle()
.zip(message.chars())
.map(|(transpose_distance, message_char)|
let transposed_char_idx = char_to_alpha_idx(message_char) + transpose_distance;
alpha_idx_to_char(transposed_char_idx % 26)
)
.collect();
This is a handful, but it works exactly as your previous variant.
Encapsulate functionality in functions
Now, my main critique point is that you don't provide a single function for your cipher. It's hard to test your functionality at the moment, so we better add one and some documentation:
/// Applies [The Alphabet Cipher] given by `keyword` to `message`.
///
/// Both `message` and `keyword` must only contain lowercase alphabetic ASCII
/// characters.
///
/// # Panics
/// Panics if any non-lowercase or non-alphabetic ASCII character is encountered.
///
/// # Examples
/// The character 'a' in the keyword won't change the input:
/// ```
/// assert_eq!(alphabet_cipher("aaaaa", "hello"), "hello");
/// ```
///
/// They keyword gets cycled if its shorter than the message:
/// ```
/// assert_eq!(alphabet_cipher("a", "hello"), "hello");
/// assert_eq!(alphabet_cipher("b", "hello"), "ifmmp");
/// assert_eq!(alphabet_cipher("ab","hello"), "hflmo");
/// assert_eq!(alphabet_cipher("ba","hello"), "iemlp");
/// ```
///
/// [The Alphabet Cipher]: https://en.wikipedia.org/wiki/The_Alphabet_Cipher
fn alphabet_cipher(keyword: &str, message: &str) -> String (offset, message_char)
The code snippets in the comments also get tested by cargo test
, but we can also add some more tests to check that our functions work, for example:
#[test]
fn alphabet_cipher_rot_a_to_z()
assert_eq!(
alphabet_cipher("abcdefghijklmnopqrstuvwxyz", "aaaaaaaaaaaaaaaaaaaaaaaaaa"),
"abcdefghijklmnopqrstuvwxyz"
);
There are more tests one can come up with, but those are left as an exercise.
User friendly interface
You use unwrap()
quite a lot. However, that leaves a user with unhelpful error messages:
$ ./cypher test
thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', ...
$ ./cypher test test12345
thread 'main' panicked at 'Bad char for char_to_alpha_idx', ...
Those error messages aren't that helpful. They're fine for toy programs, but you should view unwrap
as "I know that this will be Ok
or Some
, and it's fine to panic if it isn't, because I don't know how to continue". However, even in those cases, the end user should know how to continue. Keep that in mind when you write larger programs.
1
Wow. I'd love to see what you had to share if you weren't a beginner... Thanks for this.
â Simon Brahan
Apr 13 at 6:54
add a comment |Â
up vote
7
down vote
accepted
up vote
7
down vote
accepted
Disclaimer: I'm a Rust beginner with a background in C and C++. Now that I've lost my credibility, let's have a look at your code.
Use rustfmt, clippy and rustc
There are (at least) three helpful tools when one writes Rust code: rustfmt
, clippy
and the compiler rustc itself.
Rustfmt provides a common ground
The compiler doesn't yield any warnings, but rustfmt will change your code.
As many Rust developers use rustfmt
(or cargo fmt
, which calls the former), you should also make a habit of using it. That way others will feel right at home if they look at your code, and the same will hold for you when you look at theirs.
Clippy's warnings
clippy
on the other hand will catch common mistakes. In this case its mostly the redundant returns
in your idx
functions:
fn get_transpose_table(keyword: String) -> Vec<usize> char_to_alpha_idx(character))
.collect()
fn char_to_alpha_idx(character: char) -> usize
"abcdefghijklmnopqrstuvwxyz"
.find(character)
.expect("Bad char for char_to_alpha_idx")
fn alpha_idx_to_char(loc: usize) -> char
"abcdefghijklmnopqrstuvwxyz"
.chars()
.nth(loc)
.expect("Bad loc for alpha_idx_to_char")
There is no need to return
here. Next, you never consume the String
in get_transpose_table
, so we could borrow it instead with &String
. But in the end, we never use any String
-specific functionality, therefore we can just use &str
.
While we're at it, map(|x| func(x))
is just map(func)
, so let's change that as-well:
fn get_transpose_table(keyword: &str) -> Vec<usize>
keyword.chars().map(char_to_alpha_idx).collect()
All those small improvements were reported by clippy
.
Use T::with_capacity
if you know how many elements you will have
For your main algorithm, you should use String::with_capacity
if you know the number of characters you want to store:
let mut output = String::with_capacity(input.len());
Nothing else in your code changes.
Closures and readability
Apart from better names (message
instead of body
, body_index
or message_ index
instead of input_index
) that's all we can do with this algorithm. We could transform it into a closure:
let output: String = message
.char_indices()
.map(|(index, message_char)|
let transpose_distance = transpose_table[index % transpose_table.len()];
let transposed_char_idx = char_to_alpha_idx(message_char) + transpose_distance;
alpha_idx_to_char(transposed_char_idx % 26)
)
.collect();
but we only removed mutability. So let's get back to the drawing table. Why do we need index
? Because we want to know which usize
we should use from the transpose_table
. We would like to traverse a cycle of those values at the same time as our original message.
That's exactly what cycle()
and zip()
are for:
let output: String = transpose_table
.iter()
.cycle()
.zip(message.chars())
.map(|(transpose_distance, message_char)|
let transposed_char_idx = char_to_alpha_idx(message_char) + transpose_distance;
alpha_idx_to_char(transposed_char_idx % 26)
)
.collect();
Whether that's easier to understand than your original variant is left to you. However, we can also get rid of the intermediate Vec
:
let output: String = keyword
.chars()
.map(char_to_alpha_idx)
.cycle()
.zip(message.chars())
.map(|(transpose_distance, message_char)|
let transposed_char_idx = char_to_alpha_idx(message_char) + transpose_distance;
alpha_idx_to_char(transposed_char_idx % 26)
)
.collect();
This is a handful, but it works exactly as your previous variant.
Encapsulate functionality in functions
Now, my main critique point is that you don't provide a single function for your cipher. It's hard to test your functionality at the moment, so we better add one and some documentation:
/// Applies [The Alphabet Cipher] given by `keyword` to `message`.
///
/// Both `message` and `keyword` must only contain lowercase alphabetic ASCII
/// characters.
///
/// # Panics
/// Panics if any non-lowercase or non-alphabetic ASCII character is encountered.
///
/// # Examples
/// The character 'a' in the keyword won't change the input:
/// ```
/// assert_eq!(alphabet_cipher("aaaaa", "hello"), "hello");
/// ```
///
/// They keyword gets cycled if its shorter than the message:
/// ```
/// assert_eq!(alphabet_cipher("a", "hello"), "hello");
/// assert_eq!(alphabet_cipher("b", "hello"), "ifmmp");
/// assert_eq!(alphabet_cipher("ab","hello"), "hflmo");
/// assert_eq!(alphabet_cipher("ba","hello"), "iemlp");
/// ```
///
/// [The Alphabet Cipher]: https://en.wikipedia.org/wiki/The_Alphabet_Cipher
fn alphabet_cipher(keyword: &str, message: &str) -> String (offset, message_char)
The code snippets in the comments also get tested by cargo test
, but we can also add some more tests to check that our functions work, for example:
#[test]
fn alphabet_cipher_rot_a_to_z()
assert_eq!(
alphabet_cipher("abcdefghijklmnopqrstuvwxyz", "aaaaaaaaaaaaaaaaaaaaaaaaaa"),
"abcdefghijklmnopqrstuvwxyz"
);
There are more tests one can come up with, but those are left as an exercise.
User friendly interface
You use unwrap()
quite a lot. However, that leaves a user with unhelpful error messages:
$ ./cypher test
thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', ...
$ ./cypher test test12345
thread 'main' panicked at 'Bad char for char_to_alpha_idx', ...
Those error messages aren't that helpful. They're fine for toy programs, but you should view unwrap
as "I know that this will be Ok
or Some
, and it's fine to panic if it isn't, because I don't know how to continue". However, even in those cases, the end user should know how to continue. Keep that in mind when you write larger programs.
Disclaimer: I'm a Rust beginner with a background in C and C++. Now that I've lost my credibility, let's have a look at your code.
Use rustfmt, clippy and rustc
There are (at least) three helpful tools when one writes Rust code: rustfmt
, clippy
and the compiler rustc itself.
Rustfmt provides a common ground
The compiler doesn't yield any warnings, but rustfmt will change your code.
As many Rust developers use rustfmt
(or cargo fmt
, which calls the former), you should also make a habit of using it. That way others will feel right at home if they look at your code, and the same will hold for you when you look at theirs.
Clippy's warnings
clippy
on the other hand will catch common mistakes. In this case its mostly the redundant returns
in your idx
functions:
fn get_transpose_table(keyword: String) -> Vec<usize> char_to_alpha_idx(character))
.collect()
fn char_to_alpha_idx(character: char) -> usize
"abcdefghijklmnopqrstuvwxyz"
.find(character)
.expect("Bad char for char_to_alpha_idx")
fn alpha_idx_to_char(loc: usize) -> char
"abcdefghijklmnopqrstuvwxyz"
.chars()
.nth(loc)
.expect("Bad loc for alpha_idx_to_char")
There is no need to return
here. Next, you never consume the String
in get_transpose_table
, so we could borrow it instead with &String
. But in the end, we never use any String
-specific functionality, therefore we can just use &str
.
While we're at it, map(|x| func(x))
is just map(func)
, so let's change that as-well:
fn get_transpose_table(keyword: &str) -> Vec<usize>
keyword.chars().map(char_to_alpha_idx).collect()
All those small improvements were reported by clippy
.
Use T::with_capacity
if you know how many elements you will have
For your main algorithm, you should use String::with_capacity
if you know the number of characters you want to store:
let mut output = String::with_capacity(input.len());
Nothing else in your code changes.
Closures and readability
Apart from better names (message
instead of body
, body_index
or message_ index
instead of input_index
) that's all we can do with this algorithm. We could transform it into a closure:
let output: String = message
.char_indices()
.map(|(index, message_char)|
let transpose_distance = transpose_table[index % transpose_table.len()];
let transposed_char_idx = char_to_alpha_idx(message_char) + transpose_distance;
alpha_idx_to_char(transposed_char_idx % 26)
)
.collect();
but we only removed mutability. So let's get back to the drawing table. Why do we need index
? Because we want to know which usize
we should use from the transpose_table
. We would like to traverse a cycle of those values at the same time as our original message.
That's exactly what cycle()
and zip()
are for:
let output: String = transpose_table
.iter()
.cycle()
.zip(message.chars())
.map(|(transpose_distance, message_char)|
let transposed_char_idx = char_to_alpha_idx(message_char) + transpose_distance;
alpha_idx_to_char(transposed_char_idx % 26)
)
.collect();
Whether that's easier to understand than your original variant is left to you. However, we can also get rid of the intermediate Vec
:
let output: String = keyword
.chars()
.map(char_to_alpha_idx)
.cycle()
.zip(message.chars())
.map(|(transpose_distance, message_char)|
let transposed_char_idx = char_to_alpha_idx(message_char) + transpose_distance;
alpha_idx_to_char(transposed_char_idx % 26)
)
.collect();
This is a handful, but it works exactly as your previous variant.
Encapsulate functionality in functions
Now, my main critique point is that you don't provide a single function for your cipher. It's hard to test your functionality at the moment, so we better add one and some documentation:
/// Applies [The Alphabet Cipher] given by `keyword` to `message`.
///
/// Both `message` and `keyword` must only contain lowercase alphabetic ASCII
/// characters.
///
/// # Panics
/// Panics if any non-lowercase or non-alphabetic ASCII character is encountered.
///
/// # Examples
/// The character 'a' in the keyword won't change the input:
/// ```
/// assert_eq!(alphabet_cipher("aaaaa", "hello"), "hello");
/// ```
///
/// They keyword gets cycled if its shorter than the message:
/// ```
/// assert_eq!(alphabet_cipher("a", "hello"), "hello");
/// assert_eq!(alphabet_cipher("b", "hello"), "ifmmp");
/// assert_eq!(alphabet_cipher("ab","hello"), "hflmo");
/// assert_eq!(alphabet_cipher("ba","hello"), "iemlp");
/// ```
///
/// [The Alphabet Cipher]: https://en.wikipedia.org/wiki/The_Alphabet_Cipher
fn alphabet_cipher(keyword: &str, message: &str) -> String (offset, message_char)
The code snippets in the comments also get tested by cargo test
, but we can also add some more tests to check that our functions work, for example:
#[test]
fn alphabet_cipher_rot_a_to_z()
assert_eq!(
alphabet_cipher("abcdefghijklmnopqrstuvwxyz", "aaaaaaaaaaaaaaaaaaaaaaaaaa"),
"abcdefghijklmnopqrstuvwxyz"
);
There are more tests one can come up with, but those are left as an exercise.
User friendly interface
You use unwrap()
quite a lot. However, that leaves a user with unhelpful error messages:
$ ./cypher test
thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', ...
$ ./cypher test test12345
thread 'main' panicked at 'Bad char for char_to_alpha_idx', ...
Those error messages aren't that helpful. They're fine for toy programs, but you should view unwrap
as "I know that this will be Ok
or Some
, and it's fine to panic if it isn't, because I don't know how to continue". However, even in those cases, the end user should know how to continue. Keep that in mind when you write larger programs.
edited Apr 12 at 17:42
answered Apr 12 at 17:06
Zeta
14.3k23267
14.3k23267
1
Wow. I'd love to see what you had to share if you weren't a beginner... Thanks for this.
â Simon Brahan
Apr 13 at 6:54
add a comment |Â
1
Wow. I'd love to see what you had to share if you weren't a beginner... Thanks for this.
â Simon Brahan
Apr 13 at 6:54
1
1
Wow. I'd love to see what you had to share if you weren't a beginner... Thanks for this.
â Simon Brahan
Apr 13 at 6:54
Wow. I'd love to see what you had to share if you weren't a beginner... Thanks for this.
â Simon Brahan
Apr 13 at 6:54
add a comment |Â
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
StackExchange.ready(
function ()
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f191886%2falphabet-cypher-in-rust%23new-answer', 'question_page');
);
Post as a guest
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password