6
\$\begingroup\$

So I'm working on an implementation of the Atbash Cipher for Rust - it is an exercise on exercism.io. I come from a little C experience and found my code to be rather round about and kind of tortured. Doing str and String manipulation in Rust is something I haven't really grokked yet. It seems like this would take up fewer lines of code in C.

Below is my code - am I going about this in the right way for Rust, or am I missing some important concept or way of manipulating the data? Is this as simple as it should be?

The exercise involves getting an input &str and outputting a String, with each character changed as per the Atbash cipher, adding a space every 5 characters. Included is also a decode function. This all goes in a lib.rs.

// "Encipher" with the Atbash cipher.
pub fn encode(plain: &str) -> String {
    let mut coded: String = plain.to_string();

    coded.retain(|c| c.is_ascii_alphanumeric());
    coded.make_ascii_lowercase();

    let coded_no_spacing = String::from_utf8(
        coded
            .bytes()
            .map(|c| {
                if c.is_ascii_alphabetic() {
                    122 - c + 97
                } else {
                    c
                }
            })
            .collect(),
    )
    .unwrap();

    spacer(coded_no_spacing)
}

/// "Decipher" with the Atbash cipher.
pub fn decode(cipher: &str) -> String {
    let mut out = encode(cipher);
    out.retain(|c| c.is_ascii_alphanumeric());
    out
}

fn spacer(coded_no_spacing: String) -> String {
    let mut coded_no_spacing = coded_no_spacing.chars();

    let mut temp_char = coded_no_spacing.next();
    let mut counter = 0;
    let mut coded_with_spaces = "".to_string();
    while temp_char.is_some() {
        if counter % 5 == 0 && counter != 0 {
            coded_with_spaces.push(' ');
        }
        coded_with_spaces.push(temp_char.unwrap());
        temp_char = coded_no_spacing.next();
        counter += 1;
    }
    coded_with_spaces
}
\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! Please see What to do when someone answers. I have rolled back Rev 3 → 2 \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Oct 22 at 20:39
  • \$\begingroup\$ Is there any way for me to get a copy of the edit I made in Rev 3? I'd like to write it in to a self answer if that is a more appropriate way to share the updated code with analysis. \$\endgroup\$ – brasides Oct 22 at 20:54
  • 1
    \$\begingroup\$ Yes - check out the revision history (which is linked above the last editor's avatar, to the right of yours) \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Oct 22 at 20:55
3
\$\begingroup\$
// "Encipher" with the Atbash cipher.
pub fn encode(plain: &str) -> String {
    let mut coded: String = plain.to_string();

    coded.retain(|c| c.is_ascii_alphanumeric());
    coded.make_ascii_lowercase();

    let coded_no_spacing = String::from_utf8(
        coded
            .bytes()
            .map(|c| {
                if c.is_ascii_alphabetic() {
                    122 - c + 97

You can b'z' and b'a' to refer to the ascii codes of the letters.

                } else {
                    c
                }
            })
            .collect(),
    )
    .unwrap();

    spacer(coded_no_spacing)
}

You are mixing two different approaches here. Firstly, you make a string out of the input and then modify it. Secondly, you use an iterator over the bytes of the string. This code would be more straightforward if you just iterated over the letters.

Here is my approach:

plain
    .chars()
    .filter_map(|c| {
        if c.is_ascii_alphabetic() {
            let letter = c.to_ascii_lowercase() as u8;
            Some(char::from(b'z' - letter + b'a'))
        } else if c.is_ascii_alphanumeric() {
            Some(c)
        } else {
            None
        }
    })
    .collect()

If you haven't seen it before, the filter_map function combines filtering and mapping. The closure can return either None, to remove the element or Some(x) to provide an element in the output.

/// "Decipher" with the Atbash cipher.
pub fn decode(cipher: &str) -> String {
    let mut out = encode(cipher);
    out.retain(|c| c.is_ascii_alphanumeric());
    out
}

It took me a bit to figure out why you were filtering the chars again. But I see it is remove the spacing. It would make more sense to split the basic ciphering and into its own function so you can call that without adding the spacing. Then you wouldn't have to filter it.

fn spacer(coded_no_spacing: String) -> String {
    let mut coded_no_spacing = coded_no_spacing.chars();

    let mut temp_char = coded_no_spacing.next();
    let mut counter = 0;
    let mut coded_with_spaces = "".to_string();

I would use String::new() to create an empty string. For "extra credit", you could use String::with_capacity to reserve the correct amount of space for the string, something like:

String::with_capacity(coded_no_spacing.len() + coded_no_spacing.len() / 5);

That's dubiously worthwhile, but sometimes it can be a helpful optimization.

Onwards:

    while temp_char.is_some() {

Firstly, there is a construct you can use when you want to iterate as long as something return Some instead of None.

while let Some(temp_char) = coded_no_spacing.next()

But in this case, this is just iterating over for the chars, so you should use a forloop

for temp_char in coded_no_spacing.chars()

        if counter % 5 == 0 && counter != 0 {
            coded_with_spaces.push(' ');
        }
        coded_with_spaces.push(temp_char.unwrap());
        temp_char = coded_no_spacing.next();
        counter += 1;

Instead of counting, use the enumerate() method on iterator. It will give you an index.

    }
    coded_with_spaces
}

Here is my version:

fn spacer(coded_no_spacing: &str) -> String {
    let mut coded_with_spaces = String::new();

    for (index, char) in coded_no_spacing.chars().enumerate() {
        if index % 5 == 0 && index != 0 {
            coded_with_spaces.push(' ');
        }
        coded_with_spaces.push(char);
    }

    coded_with_spaces
}
\$\endgroup\$
  • \$\begingroup\$ Thank you so much for this detailed description of your suggestions! I incorporate your changes in the edit above, and describe why I think they are an improvement over already very helpful suggestions from user.rustlang.org in a post I made there. \$\endgroup\$ – brasides Oct 22 at 19:38
0
\$\begingroup\$

Since posting this here I received some feedback on users.rustlang.org in this post. I will post the iteration made by incorporating the suggestions in users.rustlang.org, comment on those, and add more changes by using Winston Ewert's suggestions made on this site. I'll try to use the best suggestions from both the rustlang post and Winston Ewert, but certainly appreciate feedback as to whether or not I am using the best changes. At the end I post my improved code from all suggestions.

Spoiler alert - the suggestions made here by Winston Ewert are all around improvements on the already super helpful suggestions from users.rustlang.org.

1. Code after users.rustlang.org post

/// "Encipher" with the Atbash cipher.
pub fn encode(plain: &str) -> String {
    let mut coded: String = plain.to_string();

    coded.retain(|c| c.is_ascii_alphanumeric());
    coded.make_ascii_lowercase();

    let coded_no_spacing = coded.bytes()
            .map(|c| {
                if c.is_ascii_alphabetic() {
                    (122 - c + 97) as char
                } else {
                    c as char
                }
            })
            .collect::<String>();

    let mut coded_no_spacing = coded_no_spacing.chars().enumerate();
    let mut coded_with_spaces = String::new();
    while let Some((counter, ch)) = coded_no_spacing.next() {
        if counter % 5 == 0 && counter != 0 {
            coded_with_spaces.push(' ');
        }
        coded_with_spaces.push(ch);
    }
    coded_with_spaces
}

/// "Decipher" with the Atbash cipher.
pub fn decode(cipher: &str) -> String {
    let mut out = encode(cipher);
    out.retain(|c| c.is_ascii_alphanumeric());
    out
}

2. Comments on code above

Three basic changes here:

a. let coded_no_spacing declaration changed to make use of as char instead of String::from_utf8. Also using collect::<String>() to allow code to reflect logic and avoid unnecessary unwrap from first iteration.

b. enumerate instead of declaring extraneous counter variable.

c. while let makes use of the enumerate to streamline use of temporary counter and ch.

3. Best of both

Looking at the suggestions of both posts, I think that the changes suggested here are all around improvements and have decided to incorporate these suggestions over those made at my users.rustlang.org post. These are my reasons:

a. filter_map vs separate filtering and mapping: filter_map streamlines the process required, utilizing the standard library to better effect in fewer lines with clear logic. While writing this code I saw filter_map in the documentation but couldn't get a handle on it, but thanks to the example provided in the post here I am starting to get it. Thanks!

b. for vs while let: I think the for works a little better here because it is more concise. Using while let would require one step to declare an iterator and another to set the loop running, whereas the for loop allows the iterator to be created within the construction of the loop. I'm not sure this is an important distinction but the for loop seems cleaner.


Miscellaneous

Nice tip about the little optimization with the String capacity. I guess depending on the size of the input string this could lead to many fewer runs from stack to heap and back, which is my understanding of why it might be a good one to use. I include this in my solution.

As to separating spacer into its own function and removing the filter from decode - I should have mentioned above that because I am writing this for Exercism it has to pass tests by only calling encode and decode, and so I have to leave it in. However it does seem a good suggestion to keep a separate function for spacer, and will call it in encode and keep the filter in decode.

Final Version

/// "Encipher" with the Atbash cipher.
pub fn encode(plain: &str) -> String {
    let coded_no_spacing = plain.chars()
            .filter_map(|c| {
                if c.is_ascii_alphabetic() {
                    let letter = c.to_ascii_lowercase() as u8;
                    Some(char::from(b'z' - letter + b'a'))
                } else if c.is_ascii_alphanumeric() {
                    Some(c)
                } else {
                    None
                }
            })
            .collect();

    spacer(coded_no_spacing)
}

/// "Decipher" with the Atbash cipher.
pub fn decode(cipher: &str) -> String {
    let mut out = encode(cipher);
    out.retain(|c| c.is_ascii_alphanumeric());
    out
}

/// Spacer adds one space every five characters to help encode function.
fn spacer(coded_no_spacing: String) -> String {
    let mut coded_with_spaces = String::with_capacity(
        coded_no_spacing.len() + coded_no_spacing.len() / 5);

    for (index, ch) in coded_no_spacing.chars().enumerate() {
        if index % 5 == 0 && index != 0 {
            coded_with_spaces.push(' ');
        }
        coded_with_spaces.push(ch);
    }

    coded_with_spaces
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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