11
\$\begingroup\$

I wrote a ROT13 cipher script. 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
    \$\begingroup\$ Why do you have the return inside the for loop? \$\endgroup\$ – Solomon Ucko Apr 16 '18 at 22:02
16
\$\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\$
  • \$\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 at 9:56
  • 1
    \$\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 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 at 18:13
2
\$\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
    \$\begingroup\$ You need to also write a review. Posting just code is off-topic. \$\endgroup\$ – t3chb0t Jan 19 '18 at 9:07
  • \$\begingroup\$ I just added a review. Didn't know the rule :( \$\endgroup\$ – buihdk Jan 19 '18 at 10:48
2
\$\begingroup\$

Shorter instance of str_rot13 in JavaScript.

function str_rot13(str){
    return (str+'').replace(/[a-zA-Z]/gi,function(s){
        return String.fromCharCode(s.charCodeAt(0)+(s.toLowerCase()<'n'?13:-13))
    })
}
\$\endgroup\$
0
\$\begingroup\$

What about this one?

function rot13(str) {
    return str.replace(/([A-M])|([N-Z])/g, function(m, p1, p2) {
        return String.fromCharCode(m.charCodeAt(0) + (p1 ? 13 : -13)); 
    });
}

Return the string calling a replace method where the first parameter is a regexp pattern that will match uppercase letters in A-M range or N-Z range, then, in the second parameter create a callback function passing 3 arguments: match substring, p1 = match for A-M range and p2 = match for N-Z range. Then return the new string from its code obtained from sum or substract 13 to the current chartcode.

\$\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.