6
\$\begingroup\$

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(|character| char_to_alpha_idx(character))
                  .collect();
}

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");
}
\$\endgroup\$
0

1 Answer 1

8
\$\begingroup\$

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> {
    keyword
        .chars()
        .map(|character| 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 {
    keyword
        .chars()
        .map(char_to_alpha_idx)
        .cycle()
        .zip(message.chars())
        .map(|(offset, message_char)| {
            let index = char_to_alpha_idx(message_char) + offset;
            alpha_idx_to_char(index % 26)
        })
        .collect()
}

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.

\$\endgroup\$
1
  • 2
    \$\begingroup\$ Wow. I'd love to see what you had to share if you weren't a beginner... Thanks for this. \$\endgroup\$ Apr 13, 2018 at 6:54

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.