You Didn’t “refactor” Anything

Methinks you and I are on different pages in terms of what “refactoring” means. You didn't really change much in terms of how it works, your methodology is grossly inefficient, and you missed an obvious flaw in your logic.

You just made it more verbose and "broke out" some constant values into the global (or parent) scope. Yawn.

Looking For Inefficiencies

This is something I consider an essential to refactoring and you have a glaring one. Since this is all limit based math, there is no reason for there to be two separate functions. If you allow negative and positive shifts, you can do this with just ONE function! One function can handle both encode and decode!

Next up you’re inclusion testing instead of exclusive testing. “continue” is your friend just as early returns are. More so you didn’t notice something about ASCII. Maybe it’s because I’m an assembly language guy from the late ‘70’s and early ‘90’s, but I think about ASCII transitions in binary.

As such the most efficient way to reject characters for encoding would be process of elimination bitwise. It’s why ASCII7 (characters 0..127) are laid out in rows of 32.

Finally charCodeAt is utf-16, but you’re not accounting for that in your index.

Binary Operations on ASCII, A Better Way

Thus, you could reject and continue if:

charCode & 0xFF80 // utf-8 and/or characters over 127

!(charCode & 0x0040) // character is not 64..127

!(charCode & 0x001F) // character %32 == 0 “@`"

(charCode & 0x001F) > 0x1A // character %32 > 26 “Zz”

And with binary comparisons being so blazing fast…

The character math could be simplified too.

temp = (charCode & 0x1A) + shift;
while (temp < 1) temp += 0x1A;
while (temp > 0x1A) temp -= 0x1A;

To get the final character code:

result += String.fromCharCode(temp | (charCode & 0x0060));

Restoring the top two bits to it from the original.

Hence a single routine to both cipher and decipher would read:

function cipher(text, shift) {
for (
var i = 0, result = "", code, temp;
code = text.charCodeAt(i);
) {
if (
(code & 0xFF80) || // above character code 127
(!(code & 0x0040)) || // bit 6 not set, not in "alpha rows"
(!(temp = code & 0x001F)) || // grab bottom 5 bits, empty?
(temp > 0x1A) // is > 26
) {
if (code & 0xFF00) i++; // don't forget to make utf-16 safe!
result += String.fromCharCode(code);
temp += shift;
while (temp < 1) temp += 0x1A;
while (temp > 0x1A) temp -= 0x1A;
result += String.fromCharCode(temp | (code & 0x60));
return result;
} // cipher

Which I guarantee executes many times faster. That’s a code refactor.

If you encode with a shift of -3 (like your example) to decode you just encode again by +3! One routine, goes both ways.

Oh also, avoid making variable names that are the same as the function they are inside. You want confusing, there it is.

And before some folks get their panties in a knot… YES I used comparison on assignment (aka “conditional expression”). YES I’m using var and not let/const and I did so for good reason. YES, I’m using comparison on assignment as the conditional in a for loop. BLOW ME! These things exist for a reason, don’t be afraid to use them.

Get the Medium app

A button that says 'Download on the App Store', and if clicked it will lead you to the iOS App store
A button that says 'Get it on, Google Play', and if clicked it will lead you to the Google Play store