HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Caesar cipher implementation

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
implementationcaesarcipher

Problem

I am not very familiar with JavaScript and I'am a little confused with the object oriented peculiarities of it. I am trying to create a Caesar shift using the concepts of objects, methods and properties in JavaScript. I am using an HTML form to take input from user and OnClick return the encoded cipher text to user. This is what I have. I'm pretty much sure about my logic but I guess my object creation and method calls fall through. Am I doing it right?


function Caesar(order){
this.order = order;
this.encode = encode;
function encode(input){
this.output = '';
for (var i = 0; i = 65 && this.c = 97 && this.c 

Enter Plaintext :  
Enter Shift:       --How do I get the input from here and create my caesar object and pass the constructor the shift from this text box?
 --How do I call the encode() method with input from the plaintext text box?  

Solution

Your code is reviewable, even if it does not work, from a once over:

-
Indent your code (!!), this is bad:

function Caesar(order){
this.order = order;
this.encode = encode;
function encode(input){
this.output = '';
for (var i = 0; i = 65 && this.c = 97 && this.c <= 122) this.output += String.fromCharCode((this.c - 97 + this.key) % 26 + 97);  // Lowercase
else   this.output += input.charAt(i);    
}
return answer.innerHTML= output;
 }


This is better:

function Caesar(order){
    this.order = order;
    this.encode = encode;
    function encode(input){
        this.output = '';
        for (var i = 0; i = 65 && this.c = 97 && this.c <= 122) 
                // Lowercase
                this.output += String.fromCharCode((this.c - 97 + this.key) % 26 + 97);  
            else   
                this.output += input.charAt(i);    
        }
    return answer.innerHTML= output;
}


  • Caesar is an unfortunate name



  • It does not really say what it does ( one can guess )



  • it should not start with a capital C, it is not a constructor



  • The Caesar is completely wrong



  • it puts encode in this.encode and then never uses this.encode



  • if encode were called, it set this.output but then in the end you put output in answer.innerHTML



  • you try to access answer, but you never declared it with var, much less pointed it to the answer element with getElementById



  • You hardcode magical numbers



  • Some developers know 65 is A, you should put a comment about that



  • Same for 97 , 26 and 122



  • order -> what does order do ? Should be commented, it is not obvious



  • onclick ="encode()"

Code Snippets

function Caesar(order){
this.order = order;
this.encode = encode;
function encode(input){
this.output = '';
for (var i = 0; i < input.length; i++) {
var this.c = input.charCodeAt(i);
if      (this.c >= 65 && this.c <=  90) this.output += String.fromCharCode((this.c - 65 + this.order) % 26 + 65);  // Uppercase
else if (this.c >= 97 && this.c <= 122) this.output += String.fromCharCode((this.c - 97 + this.key) % 26 + 97);  // Lowercase
else   this.output += input.charAt(i);    
}
return answer.innerHTML= output;
 }
function Caesar(order){
    this.order = order;
    this.encode = encode;
    function encode(input){
        this.output = '';
        for (var i = 0; i < input.length; i++) {
            var this.c = input.charCodeAt(i);
            if (this.c >= 65 && this.c <=  90) 
                // Uppercase
                this.output += String.fromCharCode((this.c - 65 + this.order) % 26 + 65);  
            else if (this.c >= 97 && this.c <= 122) 
                // Lowercase
                this.output += String.fromCharCode((this.c - 97 + this.key) % 26 + 97);  
            else   
                this.output += input.charAt(i);    
        }
    return answer.innerHTML= output;
}

Context

StackExchange Code Review Q#45609, answer score: 8

Revisions (0)

No revisions yet.