ROT13 JavaScript

Question

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

• Solomon Ucko: Why do you have the return inside the for loop?

Answer 1

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]}), {})

• Jasper: 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.
• Jonah: @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."
• Jasper: 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.

Answer 2

We are looking for answers that provide insightful observations about the code in the question. Answers that consist of independent solutions with no justification do not constitute a code review, and may be removed.

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.

• t3chb0t: You need to also write a review. Posting just code is off-topic.
• Khoa Bui: I just added a review. Didn't know the rule :(

Answer 3

We are looking for answers that provide insightful observations about the code in the question. Answers that consist of independent solutions with no justification do not constitute a code review, and may be removed.

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.

• Sᴀᴍ Onᴇᴌᴀ: You have presented an alternative solution, but haven't reviewed the code. Please explain your reasoning (how your solution works and why it is better than the original) so that the author and other readers can learn from your thought process. Please read Why are alternative solutions not welcome?
• jefelewis: TypeScript typings