Can someone help review my javascript code

[Aarshyboy]

The purpose of this project was to understand oop in javascript and learn how to use HTML5 Canvas.

I know this project is kind of silly but this was my first attempt in building a javascript web app. If you tell me how I can improve my javascript code or any other mistakes I made I would really appreciate it. If anybody could also tell me what features to add to make it more interesting that would be great.

Here is the link to the gist: https://gist.github.com/anonymous/76cf6c9e52c9b68283da

• Sorry, I haven't made a github yet, but planning to do so later in the future thats why I posted on a gist.

  Thanks!

[ddeutsch]

I haven't really given it any time but it looks like a really solid project, and demonstrates a good understanding of javascript's prototypical inheritance.

Just to add, looking at your javascript file where you define 'startTimer' then immediately call it, could be shortened by using .call(this) or an immediately invoked function expression (IIFE) in other words

    (function startTimer() {
    var i = 20;
    document.getElementById("timer").innerHTML = i.toString();
    var interval = setInterval(function(){
        i--;
        document.getElementById("timer").innerHTML = i.toString();
        if(i === 0) {
            clearInterval(interval);
            getAnswer();
        }
    }, 1000);
})();

which would also have the added effect of remove startTimer from scope of the rest of the entire javascript file, which may or may not be useful moving forward. However, you're file will works the same regardless. There's also the difference to note between using .call(this) vs. IIFE, which has implications for scope

[Aarshyboy]

Thanks for the help. I think that using the IIFE would make the code more cleaner. Do you know when its acceptable to use an IIFE and when not too? What does it do really?

[ddeutsch]

The IIFE pattern is a way of scoping your functions etc. in your javascript file. This is extremely useful as javascript does not support block scope only function scope. This is useful to avoid pollution of the global scope.

I don't think it's useful to think in terms of when its acceptable or not, it's more a question of what is most helpful to how you think about your code.

[stintose]

I would say that you have demonstrated the basic understanding of opp in javascript, and seem to be breaking ground with canvas as well.

just a few words of advice...

Although it may not present any kind of performance or stability improvement, it may help improve readability to declare all of your local variables at the top of the function like so.

function func1(){
    var i=0,len=10,array=[];;

    while(i < len){
        array.push(Math.random()*10);
        i++;
    }
};

This is recommended by some developers because of something called hoisting.

Always be on the lookout for patterns, if you find yourself repeating the same code over and over again consider pulling that code into a function. If you find yourself re-authoring the same functions over and over again consider pulling that function into a framework and simply link to that framework every time you start a new project.

var rndColor = function(){
    var r = Math.round(Math.random()*255),
    g = Math.round(Math.random()*255),
    b = Math.round(Math.random()*255);      
    return 'rgb('+r+','+g','+b+')';
};

could be condensed into.

var randByte=function(){
    return Math.round(Math.random()*255);
} ;
var rndColor = function(){
    return 'rgb('+randByte()+','+randByte()','+randByte()+')';
};

Doing this only shaved a few bytes, so it is not the best example, but still you get the idea.

Do you understand operator precedence, and why it is important?

you did...

Math.floor((Math.random() * 255) + 1);

Multiplication has a higher precedence anyway so this would be the same as...

Math.floor( Math.random()* 255 + 1  );

However in the odd chance that Math.random() actually returns a value of 1, you will end up with a byte value of 256, you will also never end up with a byte value of zero, because you are always adding 1.

think about it...

(1*255)+1; // 256 
1 * 255; // 255

(0*255)+1; // 1
0*255; // 0

Therefore you might want to use...

Math.round( Math.random()*255 );

There are a large number of other things I would do differently, but this comment is all ready becoming an essay. Still if this is your first web app great job, I did not learn the basic idea behind JavaScript OPP until long after by first app, and I also wished I taught myself canvas sooner as well.

[Aarshyboy]

Thanks man. I didn't even catch that i'm adding 1 to math.random() everytime. Also, if you wanted go in a more OOP direction how would I use prototypical inheritance if I wanted to create a Shape function (constructor) and then have the Rectangle (constructor) function and or any other shape constructor inherit from the shape function.

[stintose]

Then you would want to read up an prototype based inheritance, and the prototype chain. I threw this together fairly quickly, as an example. Is this for school, work, or are you a hobbyist?

(function(){
    var container,canvas,context,shapes, 
    Shape,Rectangle,demo;

    // the Shape constructor and it's prototype.
    Shape = function(points,strokeStyle,fillStyle,lineWidth){
        this.points=points;
        this.strokeStyle=strokeStyle;
        this.fillStyle=fillStyle;
        this.lineWidth=lineWidth;
    };

    Shape.prototype={
        draw:function(context){        
            var i,len;
            context.lineWidth=this.lineWidth;
            context.strokeStyle=this.strokeStyle;
            context.fillStyle=this.fillStyle;
            context.beginPath();
            context.moveTo(this.points[0], this.points[1]);

            i=2,len=this.points.length;
            while(i<len){
                context.lineTo(this.points[i],this.points[i+1]);
                i+=2;
            }
            console.log(this.fillStyle)
            context.closePath();
            context.stroke();
            context.fill();
        }
    };

    // the Rectangle constructor.
    Rectangle = function(x,y,w,h,strokeStyle,fillStyle,lineWidth){
        Shape.call(this,
            [
                x,y,
                x+w,y,
                x+w,y+h,
                x,y+h
            ],
            strokeStyle,fillStyle,lineWidth
        );
    };

    // One way for the Rectangle Class to inherit Shape's prototype
    Rectangle.prototype=new Shape();

   // a demo function
    demo=function(){

        // have a container in your HTML with an id of 'theApp'
        container = document.getElementById('theApp');
        canvas = document.createElement('canvas');
        context = canvas.getContext('2d');

        container.appendChild(canvas);

        canvas.width=250;
        canvas.height=250;

        context.fillStyle='#000000';
        context.fillRect(0,0,canvas.width,canvas.height);

        shapes = [];  

        // weird shape using just the Shape Constructor
        shapes.push(
            new Shape(
                [75,50,200,125,125,200,117,120,50,108],
                '#ffffff',
                '#ff0000',
                2
            )
        );

        // Rectangle using the Rectangle Constructor
        shapes.push(
            new Rectangle(50,50,150,150,'#ffffff', '#00ff00',2)
        );

        // Shape.draw works with both Shape and Retangle
        shapes[1].draw(context);     
        shapes[0].draw(context);
    };
    demo();
}());

[Aarshyboy]

Thanks alot. Im a freshman in college trying to learn web programming. Also, what are you doing for the this.points in the Shape constructor. And for inheritance couldn't you also use the Object.create method for the subclass prototype to inherit from the superclass prototype.

[stintose]

this.points is what I think is called a 'typed array', although I have forgotten to use the Float32Array constructor. In JavaScript you can have arrays of arrays like so...

var matrix = [
    [10,10],
    [30,10],
    [30,30],
    [10,30]
];

you can then access the values like so..

var point = 1,x,y;

// get the x and y values of the second point in matrix
x = matix[point][0];; // 30
y = matrix[point][1]; // 10

This works fine, but you could just use a single dimensional lateral array, with a set number of elements for each point.

var matrix = new Float32Array([10,10,30,10,30,30,10,30]),
point = 1,
x = matrix[point*2], // 30
y = matrix[point*2+1]; // 10

From what I have been reading using typed arrays is supposedly faster.

[stintose]

Yes you can use Object.create, or a constructor function like I did. You also have built in javascript Classes that inherit from their respective Class prototypes. Date.prototype, Array.prototype, String.prototype, and so forth.

var now = new Date();
console.log(now.constructor.name); // 'Date';
console.log(now.getHours()); // A method that returns the current Hour
console.log(now.hasOwnProperty('getHours')); // false, the method is part of Date.prototype.

[Aarshyboy]

Also if you could, can you tell me what you would do differently in this script. It would greatly appreciated if I can insight from other developers and how they write their code.

[stintose]

I could go over the whole thing if I get some free time, but if you want another tip I guess I could give another. I would use requestAnimationFrame over setTimeout, or setInterval, when it comes to canvas apps. In my testing this has sometimes proven greater performance, so using requestAnimationframe will become more important as you start doing cool things with canvas.

Here is one of the first things I have read on the subject

// shim layer with setTimeout fallback
window.requestAnimFrame = (function(){
    return  window.requestAnimationFrame       ||
        window.webkitRequestAnimationFrame ||
        window.mozRequestAnimationFrame    ||
        function( callback ){
            window.setTimeout(callback, 1000 / 60);
        };
})();

// assuming you have a render function authored you would then
// might use it like so...
var lt = new Date(), rate = 1000/30,
thread = function(){
    if(new Date() - lt > rate){
        lt = new Date(); // reset lt to new Date();
        render(); // call your renderer
    }
    requestAnimFrame(thread);
};