16
\$\begingroup\$

I wrote a ROT13 cipher script rotating uppercase letters only. Can this function be improved?

ROT13 Wikipedia article

function rot13(str) {

var codeA = "A".charCodeAt(0);
var codeN = "N".charCodeAt(0);
var codeZ = "Z".charCodeAt(0);
var newArr = [];

for(var i =0; i<str.length; i++){
    var code = str.charCodeAt(i);
    if(code>=codeA && code<=codeZ){
        if(code>=codeN)
            newArr.push(String.fromCharCode(code-13));
        else
            newArr.push(String.fromCharCode(code+13));
    }else{
        newArr.push(str[i]);}
    }
     return newArr.join("");
}
\$\endgroup\$
1
  • 2
    \$\begingroup\$ Why do you have the return inside the for loop? \$\endgroup\$ Apr 16 '18 at 22:02
23
\$\begingroup\$

The mapping is so simple, I prefer to do a more literal implementation of the wiki explanation:

function rot13(str) {
  var input     = 'ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz';
  var output    = 'NOPQRSTUVWXYZABCDEFGHIJKLMnopqrstuvwxyzabcdefghijklm';
  var index     = x => input.indexOf(x);
  var translate = x => index(x) > -1 ? output[index(x)] : x;
  return str.split('').map(translate).join('');
}

Here the code is expressing just two rules:

  1. If it's a non-letter, don't translate it.
  2. If it's a letter, translate it by mapping its physical position in the input to its physical position in the output.

This avoids charCodes, loops, and nested if statements, all of which are implementation noise and distract from the essence of the program.

While this still executes in O(n), the constant factor on n is high, because in the worst case we have to look through 52 input letters to find each character. In practice, however, this makes very little difference: https://jsperf.com/rot13comparison

Even Faster Variation

Nevertheless, it's possible to avoid the high constant factor on n if you were in a situation where squeezing out every last drop of performance mattered. And indeed, you could probably squeeze out even more by replacing the split/map with a for loop. But again, these kinds of optimizations are rarely needed, and concentrating on clean code should usually take precedence.

Here's a slight variation on the same theme, which creates a lookup dictionary between the input and output letters using an object, and then uses that object to do the translation:

  function rot13Fast(str) { 
    return str.split('').map(x => rot13Fast.lookup[x] || x).join('')
  }
  rot13Fast.input  = 'ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz'.split('')
  rot13Fast.output = 'NOPQRSTUVWXYZABCDEFGHIJKLMnopqrstuvwxyzabcdefghijklm'.split('')
  rot13Fast.lookup = rot13Fast.input.reduce((m,k,i) => Object.assign(m, {[k]: rot13Fast.output[i]}), {})
\$\endgroup\$
3
  • \$\begingroup\$ While I like your general approach, suggesting an O(n^2) algorithm for something that can (and was previously) done in O(n) is a bad idea. Having an O(n) variation at the end of the answer redeems it a little, but without explaining its benefit, it may not be clear why someone would use the variation (and in my opinion, it should have been the main solution either way). Also note that the variation generates the dictionary every time it is invoked, even though it's the same every time. \$\endgroup\$
    – Jasper
    Jan 28 '19 at 9:56
  • 2
    \$\begingroup\$ @Jasper Both versions are O(n), not O(n^2). The constant factor on n in the first variation is 52, and while perhaps worth pointing out (see my edit), this will rarely matter in practice. Simpler code trumps micro-optimizations in most cases unless you have a proven need for them. Lots of overly complex code is justified in the name of "efficiency." \$\endgroup\$
    – Jonah
    Jan 28 '19 at 14:20
  • \$\begingroup\$ Ah yes, you're right, the first is indeed O(n) as well, my bad on that one. And while I fully agree micro-optimizations when unwarranted are a Really Bad Thing, I'd describe this more as a "mini-optimization", relevant in significantly more situations and really not that bad when it comes to clean code. That said, you're absolutely right, it's not as bad as I thought and it's definitely not necessary to go for the second solution in every situation. \$\endgroup\$
    – Jasper
    Jan 28 '19 at 18:13
4
\$\begingroup\$
function rot13(str) {
  return str.replace(/([A-M])|([a-m]])|([N-Z])|([n-z])/g, function(match, p1, p2, p3, p4) {
    switch(match) {
      case p1:
      case p2:     
        return String.fromCharCode(match.charCodeAt(0) + 13);
      case p3:
      case p4:
        return String.fromCharCode(match.charCodeAt(0) - 13);
    }
  });
}

This code should work for both upper cases and lower cases. p1 contains all chars that match [A-M], p2 contains all chars that match [a-m], p3 contains all chars that match [N-Z], and p4 contains all chars that match [n-z]. For chars that are in range A/a to M/m, shift char-code up by 13 (+). For chars that are in range N/n to Z/z, shift char-code down by 13 (-).

More on the replace() function can be found here.

\$\endgroup\$
2
  • 2
    \$\begingroup\$ You need to also write a review. Posting just code is off-topic. \$\endgroup\$
    – t3chb0t
    Jan 19 '18 at 9:07
  • 2
    \$\begingroup\$ I just added a review. Didn't know the rule :( \$\endgroup\$
    – Khoa Bui
    Jan 19 '18 at 10:48
0
\$\begingroup\$

2020 TypeScript Version:

  • A shift argument must be given, but you can choose 13 for rot13 or any other number.
// Caesar Cipher
const caesarCipher = (string: string, shift: number) => {
  // Alphabet
  const alphabet: string = 'ABCDEFGHIJKLMNOPQRSTUVWXYZ';

  // Encoded Text
  let encodedText: string = '';

  // Adjust Shift (Over 26 Characters)
  if (shift > 26) {
    // Assign Remainder As Shift
    shift = shift % 26;
  }

  // Iterate Over Data
  let i: number = 0;
  while (i < string.length) {
    // Valid Alphabet Characters
    if (alphabet.indexOf(string[i]) !== -1) {
      // Find Alphabet Index
      const alphabetIndex: number = alphabet.indexOf((string[i]).toUpperCase());

      // Alphabet Index Is In Alphabet Range
      if (alphabet[alphabetIndex + shift]) {
        // Append To String
        encodedText += alphabet[alphabetIndex + shift];
      }
      // Alphabet Index Out Of Range (Adjust Alphabet By 26 Characters)
      else {
        // Append To String
        encodedText += alphabet[alphabetIndex + shift - 26];
      }
    }
    // Special Characters
    else {
      // Append To String
      encodedText += string[i];

    }

    // Increase I
    i++;
  }

  return encodedText;
};

Example #1:

console.log(caesarCipher('ABCDEFGHIJKLMNOPQRSTUVWXYZ', 2));

Output:
CDEFGHIJKLMNOPQRSTUVWXYZAB

Example #2:

console.log(caesarCipher('GUR DHVPX OEBJA QBT WHZCRQ BIRE GUR YNML SBK.', 26 + 13));

Output:
THE QUICK BROWN DOG JUMPED OVER THE LAZY FOX.
```
\$\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.