3
\$\begingroup\$

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?

<head>
<script>
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;
 } 
</script></head>
<body>

<form>
Enter Plaintext : <input type = "text" name = "plaintext" id = "plaintext"> 
Enter Shift:      <input type = "text" name = "shift" id = "shift"><br> --How do I get the input from here and create my caesar object and pass the constructor the shift from this text box?
<input type="button" value="Encode" onclick ="encode()"> --How do I call the encode() method with input from the plaintext text box?  
</form>
</body></html>
Improve this question
\$\endgroup\$
3
  • \$\begingroup\$ Welcome! Does your code works ? \$\endgroup\$
    – Marc-Andre
    Mar 28, 2014 at 14:11
  • \$\begingroup\$ The encode function works independently. However, it doesn't run when I try calling it as a method. \$\endgroup\$
    – an91
    Mar 28, 2014 at 14:28
  • \$\begingroup\$ Welcome to Code Review! It seems like you have misunderstood what this site is about. We help with improving the cleanliness of existing, working code. We are not the site to ask for help in fixing or changing what your code does. We love helping people do the same thing in a cleaner way! \$\endgroup\$
    – Simon Forsberg
    Mar 28, 2014 at 15:58

1 Answer 1

Reset to default
8
\$\begingroup\$

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 < 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;
 }

    This is better:

    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;
}
  • 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()" <- This is very old skool ( bad ), the correct way is using an addEventListener for load and in there use an addEventListener for click
Improve this answer
\$\endgroup\$
2
  • \$\begingroup\$ Thank you for your comments. However, I'd like to point out that Caesar is a constructor function. I want to be able to create Caesar objects with 'new' and initialize them with 'order'. Is it not the right way to create a constructor function? \$\endgroup\$
    – an91
    Mar 28, 2014 at 14:49
  • \$\begingroup\$ It is the right way, but the constructor should not return answer.innerHTML= output; in that case \$\endgroup\$
    – konijn
    Mar 28, 2014 at 15:44

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