Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I have made a small script for encrypting data. Now I'm wondering how efficient it is for normal users who aren't programmers and for people who are programmers.

I mean if you don't have this script there is no way to find out what the text is right?

function cipher(message, action) {
  var text = message;
  var encrypted = "";

  for(var i = 0; i < text.length; i++) {
    var ASCII = text[i].charCodeAt(0);
    var n = null;

    if(i % 2 == 0) {
      n = action == 'encrypt' ? ASCII + 4 : ASCII - 4;
    }

    else if(i % 2 == 1) {
      n = action == 'encrypt' ? ASCII + 7 : ASCII - 7;
    }

    var s = String.fromCharCode(n);

        encrypted += s;;
  }
   return encrypted;
}


console.log(cipher("Hello World", 'encrypt'))

If you were to make it more secure, what would you add? I was thinking of a complete custom random mapping for each letter but retrieving it after decryption is something I can't figure out yet.

share|improve this question
    
First rule of cryptography: Don't roll your own crypto. – Mast May 4 at 9:10
    
@Mast haha ya. Im just learning so its ok : ) – Asperger May 4 at 9:27
up vote 2 down vote accepted

This is a simple variant of the , namely a with the fixed key 'EH'. Any variant of these ciphers is considered a toy-grade cipher that is trivially crackable if the attacker has gathered enough ciphertext to analyze the letter frequencies. There is no way to make this class of ciphers secure, except to allow an insanely long and random key to be used, in which case it becomes a one-time pad.

If, despite acknowledging that it cannot be made secure, you still wanted to try to make some improvements to this code as a learning exercise, you could…

  • Allow the key to be specified as a parameter rather than hard-coded as alternating between +4 and +7
  • Eliminate the text variable
  • Write var ASCII = text[i].charCodeAt(0); as var unicode = message.charCodeAt(i);
  • Avoid repeated string concatenation with encrypted += s;;. Since strings in JavaScript are immutable, you are actually creating a new string and copying the entire old string each time you want to append one character. Instead, you should use Array.join():

    var encrypted = new Array(message.length);
    for (var i = 0; i < message.length; i++) {
        …
        encrypted[i] = …;
    }
    return encrypted.join('');
    
share|improve this answer
    
Minor quibble: Array.join is undefined; Perhaps Array.prototype.join or Array#join is more correct. – Cᴏɴᴏʀ O'Bʀɪᴇɴ May 4 at 11:18

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.