Caesars Cipher Project Feedback[spoiler]

JavaScript
#1

Just wondering if anyone has any advice or feedback for me on this project. Is this a good solution or is there anything that could have been shortened ? Any advice or thoughts are appreciated thanks.

function rot13(str) {
  
let rot = str.split('');

let ciph = ['A','B','C','D','E','F','G','H','I','J','K','L','M','N','O','P','Q','R','S','T','U','V','W','X','Y','Z'];
let cipher = [];
  for(let i = ciph.length-1; i >= 0; i--) {
    cipher.push(ciph[i])
  };

  for(let i = 0; i < rot.length; i++) {
      let holder = []
      if(ciph.indexOf(rot[i]) - 13 > -1) {
      holder = ciph.indexOf(rot[i]) - 13;
      rot[i] = ciph[holder]
    } else if(cipher.indexOf(rot[i]) - 13 > -1) {
      holder = cipher.indexOf(rot[i]) - 13;
      rot[i] = cipher[holder]
    };
  };  
return rot.join('')
};

rot13("SERR PBQR PNZC");
#2

Hi @m4rk!

I am going to move this to the javascript section instead. You might get more responses there.

#3

That’s an interesting way to solve the problem! It took me a while to figure out how it worked, but it obviously does.

As far as suggestions, you initialize holder as an empty array, but then use it as an integer, so that is misleading. You can just say

let holder;

and not give it a default value, or even initialize it within each of the blocks.

If you really want to shorten things, you could remove “holder” altogether and just write

  rot[i] = cipher[cipher.indexOf(rot[i]) - 13]

You could also use charCodeAt and fromCharCode instead of creating your own array…

Finally, in the interests of readability, it’s good to give your variables more descriptive names, like cipher and cipherReversed.

Happy New Year!

#4

Thank you for the response! I didn’t realize I could write holder that way, and I thought there was a way I could rewrite that to shorten the loop but couldn’t think of it.

It took a while to figure this one out and write it the way and I did. I thought about using charCodeAt when I was almost done but I wanted to try to finish it this way since I had spent the time on it.

I also hadn’t thought too much about readability, I’ve been making them short so it’s easier to type but that’s probably something I should get in the habit of.

Thank you for the feedback, Happy New Year!

1 Like
#5

Not saying this is the right solution or the best solution, but here’s mine.

I do use fromCharCode and charCodeAt, but i also tried to make it as generic as possible. It should work with any offset, upper or lower.

And the big reason to show this, though, is commenting. More then anything, while I’m learning, i use comments to tell myself what i expect, and document what actually happens.

Cipher on repl

1 Like
#6

Thanks for the response. I see what you mean, I sometimes try to make a comment of an overview of what needs to be done and then do a mental overview of the whole thing when I’m finished. But making longer notes like this would probably help a lot in the learning process, and help if I need to come back to it in the future. I’ll try to do this more, thanks for the advice.

#7

In addition to what has already been mentioned, I would rewrite this as follows:

let ciph = ['A','B','C','D','E','F','G','H','I','J','K','L','M','N','O','P','Q','R','S','T','U','V','W','X','Y','Z'];
let ciphReversed = [...ciph].reverse();

Note that the .reverse method changes the original array. That’s why you first create a new empty array [], then write the values of ciph into it with the ... spread syntax, and finally reverse that new array.

1 Like
#8

Thanks for that. I tried reverse method first and couldn’t get it to work without mutation but I see how this does it correctly.