CaesarsCipher Code Quality Concern

Question

I have been following the FreeCodeCamp, and I am done with the basic algorithm challenges. The last one (CaesarsCipher) troubled me a bit, though I had done a similar one without much problem in C, for CS50 course.

Finally I made it work, but at the moment I am self-critical about "How?" I made it work and the quality of my code. I found the basic solution provided quite hard to understand and it looks way advanced than my code. I am posting my code here with the hope of receiving some feedback on it and finding out if am I doing something wrong/should be concerned about improving quality before moving on to new topics and challenges. (such as writing style or the functions i use/ the way I handle the problem)

function rot13(str) { // LBH QVQ VG!

  //defining an isAlpha function to skip the non alpha chars

  var isAlpha = function(char){
    return typeof char==="string" && char.length == 1 && char <= "Z" && char >="A";
   };

  //two empty arrays two use later

  var shifted = [];
  var decoded= [];

  /*transform the encoded chars to ascii numbers then shift by 13 
  and copy the decoded char to another array
  */

  for(var i = 0; i < str.length; i++){

    if(isAlpha(str[i]) && str[i].charCodeAt() < 78) {

      shifted[i] = str[i].charCodeAt()+13;

      decoded.push(String.fromCharCode(shifted[i]));
    }

    else if(isAlpha(str[i]) && str[i].charCodeAt() >= 78){

      shifted[i] = str[i].charCodeAt() - 13;

      decoded.push(String.fromCharCode(shifted[i]));
    }

    else{

      decoded.push(str[i]);
    }
  }

  //lastly merge the decoded chars into a string

  var last = decoded.join("");

  return last;



}

// Change the inputs below to test
rot13("SERR PBQR PNZC");

Answer 1

var isAlpha = function(char){
    return typeof char==="string" && char.length == 1 && char <= "Z" && char >="A";
};

I like this. I like the careful type and length checking, and the comparison with strings rather than weird numeric codes. I would suggest first that it could do with handling lower case letters as well. Second, it seems like a moderately useful general purpose function, and not something that can only be used within the scope of a rot13 call. I'd be inclined to move it out.

---

Your main case analysis works (Except for not handling lower case numbers) but it could probably be clearer. I would suggest

if(isAlpha(str[i])) {
    if (str[i].charCodeAt() < 78) {
        ...
    } else {
        ...
    }
} else { 
    ...
}

This makes it clear that you have handled every possible case. In my opinion it also makes it clearer what the structure of the conditional is.

It would be even clearer what you're testing if 78 wasn't sitting there as a magic number. Using "N".charCodeAt() or even better "A".charCodeAt() + 13 would help. If concerned about the performance penatly of doing the extra function call in the check, make it a constant outside the loop.

---

var shifted = [];
var decoded= [];

I would suggest that you preallocate the length of your decoded array to be the length of your input string. This improves efficiency because Javascript knows to pack it tightly, and knows exactly how big it is going to get. It also improves readability by providing contextual clues. (i.e. that it's associated with str

I would completely delete shifted. You don't need an array to hold on to intermediate values that are immediately finished and placed in decoded. If you don't want to put the whole thing into one line, use a single scalar variable instead of an array.

---

// Change the inputs below to test
rot13("SERR PBQR PNZC");

It is good to test code! And it's nice when CodeReview questions actually come with evidence of testing too. I would suggest that a slightly more pedantic test case might be suitable. I'd want to use something with all classes of characters: upper & lower case, numbers, spaces, and assorted symbols. In particular I'd make sure that the test case contains both "A" and "Z" as well as "@" and "[" These are just outside of the letters range in ASCII, so good for checking for Off By One errors. It would also be quite nice to express the target output so that we know whether it's worked!

(In a proper testing framework that would be in code, but a comment probably suffices here.)

---

In terms of naming, it broadly seems fine. I would probably choose a different name for str, possibly cipherText.

---

In terms of formatting things are a bit all over the place. I don't have strong opinions on JavaScript formatting style, but whatever style you pick it's worth sticking to consistently. Personally there are too many blank lines for my liking; in particular a blank line between the end of an if clause and its associated else clause is really jarring!