Rotate every letter in string by K places

Question

The challenge: Given a string S and a fixed number K, rotate every letter in S by K places (if reach the end of the alphabet, go back to the beginning). Letters should remain the same case. Symbols should remain unencrypted.

Constraints: K is between 0 and 100. S is a valid ASCII string without spaces, between 1 and 100 letters long.

Input: The first line contains an integer, N , which is the length of the unencrypted string. The second line contains the unencrypted string, S. The third line contains the integer encryption key, K, which is the number of letters to rotate.

I'd appreciate any comments and suggestions as to how my code can be improved.

var lowerString = "abcdefghijklmnopqrstuvwxyz";
var lower = lowerString.split("");

var upperString = "ABCDEFGHIJKLMNOPQRSTUVWXYZ";
var upper = upperString.split("");

function main() {
    var n = parseInt(readLine());
    var s = readLine();
    var k = parseInt(readLine());

    printCipher(s, k);
}

function printCipher(str, key){
    var newStr = "";
    str.split("").forEach(function(elem, index){
        var changedItem = cipher(elem, key);
        newStr += changedItem;
    });
    console.log(newStr);
}

function cipher(item, key){
    var arrToUse = null;
    if(lower.indexOf(item) >= 0){
        arrToUse = lower;
    }
    else if (upper.indexOf(item) >= 0){
        arrToUse = upper;       
    }
    if(arrToUse !== null){
        var pos = arrToUse.indexOf(item);
        var newPos = getCipherPosition(pos, key);
        return arrToUse[newPos];
    }
    return item;
}

function getCipherPosition(pos, key){
    const ALPHA_COUNT = 26;
    if(key > ALPHA_COUNT){
        key = key % ALPHA_COUNT;
    }

    if(pos + key >= ALPHA_COUNT){
       return key + pos - ALPHA_COUNT;
    }
    return key + pos;
}

Answer 1

Simplify

Many elements are overcomplicated. For example this function:

function getCipherPosition(pos, key){
    const ALPHA_COUNT = 26;
    if(key > ALPHA_COUNT){
        key = key % ALPHA_COUNT;
    }

    if(pos + key >= ALPHA_COUNT){
       return key + pos - ALPHA_COUNT;
    }
    return key + pos;
}

This is equivalent to this:

function getCipherPosition(pos, key) {
    return (pos + key) % 26;
}

Perhaps you were trying to avoid using the modulo operator % if not necessary. It's true that modulo is usually not a cheap operation, and JavaScript certainly doesn't make it cheap. But this is premature optimization. The single line of code is a lot easier to read, and therefore preferred.

---

Another opportunity for simplification is this function:

function printCipher(str, key){
    var newStr = "";
    str.split("").forEach(function(elem, index){
        var changedItem = cipher(elem, key);
        newStr += changedItem;
    });
    console.log(newStr);
}

I propose this alternative:

function cipherStr(str, key) {
    return str.split('').map(function(e) { return cipher(e, key); }).join('');
}

That is:

• String concatenation in a loop is typically slow and awkward in most languages
• Mapping a bunch of values to some other values begs for using the map function
• The shorter alternative is easier to read and understand
• A function should have a single responsibility. printCipher does more than just print, it makes the cipher and then prints it. It should be split up.

---

Lastly, the cipher function can be also simplified a little bit. After checking if the character doesn't match the if and else if for lower and upper case, you could return immediately, so the rest of the code would become flatter, and easy to read.

var lower = "abcdefghijklmnopqrstuvwxyz";
var upper = "ABCDEFGHIJKLMNOPQRSTUVWXYZ";

function cipher(c, key) {
    var alphabet, pos, newPos;

    if (lower.indexOf(c) >= 0) {
        alphabet = lower;
    } else if (upper.indexOf(c) >= 0) {
        alphabet = upper;       
    } else {
        return c;
    }

    pos = alphabet.indexOf(c);
    newPos = getCipherPosition(pos, key);
    return alphabet.charAt(newPos);
}

I also changed a few other things:

• There's no need to have lower and upper as arrays, they can be left as strings. Strings also have indexOf, and you can use charAt instead of subscripting.
• In JavaScript it's (sadly) recommended to declare variables at the top of the function, because even if you write var later, the variable is live anyway in the entire method.

Performance

The cipher function has a bit of a performance issue. Whether you use an array or a string for the alphabet, indexOf does a linear search to find an element, an \$O(n)\$ operation. This is inevitable when the alphabet contains arbitrary elements, but in your implementation it's a simple English alphabet. By converting the conditions to a range check, the number of comparisons can be greatly reduced:

    if ('a' <= c && c <= 'z') {
        alphabet = lower;
    } else if ('A' <= c && c <= 'Z') {
        alphabet = upper;

Answer 2

if the only thing you need to encrypt in the string are the letters, you can be more concise by using only one loop doing something like:

if c >= 'a' && c <= 'z' {
    a = (((c-'a')+K)%26)+'a'
} else if c>= 'A' && c <= 'Z' {
    a = (((c-'A')+K)%26)+'A'
} else {
    a = c
}

with c being the char from the original string and a the char to add to the encrypted string.