Alphabet cypher in Rust

The name of the pictureThe name of the pictureThe name of the pictureClash 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");







share|improve this question



























    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");







    share|improve this question























      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");







      share|improve this question













      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");









      share|improve this question












      share|improve this question




      share|improve this question








      edited Apr 13 at 6:46
























      asked Apr 12 at 14:57









      Simon Brahan

      1523




      1523




















          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.






          share|improve this answer



















          • 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










          Your Answer




          StackExchange.ifUsing("editor", function ()
          return StackExchange.using("mathjaxEditing", function ()
          StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix)
          StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
          );
          );
          , "mathjax-editing");

          StackExchange.ifUsing("editor", function ()
          StackExchange.using("externalEditor", function ()
          StackExchange.using("snippets", function ()
          StackExchange.snippets.init();
          );
          );
          , "code-snippets");

          StackExchange.ready(function()
          var channelOptions =
          tags: "".split(" "),
          id: "196"
          ;
          initTagRenderer("".split(" "), "".split(" "), channelOptions);

          StackExchange.using("externalEditor", function()
          // Have to fire editor after snippets, if snippets enabled
          if (StackExchange.settings.snippets.snippetsEnabled)
          StackExchange.using("snippets", function()
          createEditor();
          );

          else
          createEditor();

          );

          function createEditor()
          StackExchange.prepareEditor(
          heartbeatType: 'answer',
          convertImagesToLinks: false,
          noModals: false,
          showLowRepImageUploadWarning: true,
          reputationToPostImages: null,
          bindNavPrevention: true,
          postfix: "",
          onDemand: true,
          discardSelector: ".discard-answer"
          ,immediatelyShowMarkdownHelp:true
          );



          );








           

          draft saved


          draft discarded


















          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






























          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.






          share|improve this answer



















          • 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














          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.






          share|improve this answer



















          • 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












          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.






          share|improve this answer















          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.







          share|improve this answer















          share|improve this answer



          share|improve this answer








          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












          • 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












           

          draft saved


          draft discarded


























           


          draft saved


          draft discarded














          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













































































          Popular posts from this blog

          Greedy Best First Search implementation in Rust

          Function to Return a JSON Like Objects Using VBA Collections and Arrays

          C++11 CLH Lock Implementation