0
\$\begingroup\$

I am very new to JavaScript and was wondering what kind of process or design pattern is most common among JavaScript developers.

So far I have following code

index.html

<!DOCTYPE html>
<html lang="en">

    <head>
        <meta charset="utf-8"/>
        <title>Interactive Guide</title>

        <!--------------------------CSS/ FONT ASSETS------------------------->
        <link href='http://fonts.googleapis.com/css?family=Open+Sans' rel='stylesheet' type='text/css'>
        <link href='//fonts.googleapis.com/css?family=Open+Sans' rel='stylesheet' type='text/css'>
        <link rel="stylesheet" type="text/css" href="CSS/mainStyle.css">
        <!------------------------------------------------------------------->


        <!----------------------------JAVASCRIPT ASSETS---------------------->
        <script src="http://ajax.googleapis.com/ajax/libs/jquery/1.5.2/jquery.min.js"></script>
        <script src="//ajax.googleapis.com/ajax/libs/jquery/1.5.2/jquery.min.js"></script>
        <script src="./JavaScript/utilities.js"></script>
        <script src="./JavaScript/mainScript.js"></script>
        <!--[if lt IE 9]>
        <script src="http://html5shiv.googlecode.com/svn/trunk/html5.js"></script>
        <![endif]-->
        <!-------------------------------------------------------------------->

    </head>

    <body>
    <div id="wrap"> 
        <div class="upper">
            <div id="heading1">Copyright and Plagiarism</div>
            <div id="time"></div>
        </div>

        <div class="content">
            <canvas id="myCanvas" width="1024" height="550">Get support for canvas- go and get Chrome dude!</canvas>
            <!--<div id="rect"></div>-->
        </div>

        <footer>
            Ankur Sharma - Copyright 2013
        </footer>
    </div>  
    </body>
</html>

mainScript.js

window.addEventListener('load', windowLoaded, false);

function windowLoaded()
{
    updateClock();
    iniScene();
}

function iniScene()
{
    var canvas = getCanvas();
    iniInterface(canvas.context, canvas.canvasH, canvas.canvasW);
}

utilities.js

//---------------------------------UTILITY FUNCTIONS------------------------------

function getCanvas()
{
    if(canvas)
    {
        return canvas;
    }
    else
    {
        var canvas = {};
        canvas.node = document.getElementById("myCanvas");
        canvas.context = canvas.node.getContext('2d');
        canvas.canvasH = canvas.node.height || 500;
        canvas.canvasW = canvas.node.width || 800;
        return canvas;
    }
}

function iniInterface(ctx, canvasH, canvasW)
{   
    drawLine(ctx, 50, canvasH-50, canvasW-50, canvasH-50, 'rgb(255, 255, 255)', 'rgb(255, 255, 255)');
}

//-------------------------------OBJECTS----------------------------//



//-------------------------------DRAW FUNCTIONS---------------------//
function drawLine(ctx, sX, sY, eX, eY, sRGB, fRGB, lWidth, capStyle)
{
    ctx.beginPath();
    ctx.moveTo(sX, sY);
    ctx.lineTo(eX, eY);
    ctx.lineWidth = lWidth||5;
    ctx.strokeStyle = 'rgb(49, 129, 48)';
    ctx.lineCap = 'round';
    ctx.stroke();
    ctx.closePath();
}

 function loadImage(ctx, src, x, y, width, height)
 {
    var img = new Image();
    img.onload = function()
    {
        ctx.drawImage(img, x, y, width, height);
    };
    img.src = src;
 }

 //---------------------------------TIME---------------//

 function updateClock()
{
    var currentTime = new Date();
    var hours = currentTime.getHours();
    var minutes = currentTime.getMinutes();
    var seconds = currentTime.getSeconds();

    if(minutes<10)
    {
        minutes = "0"+minutes;
    }
    if(seconds<10)
    {
        seconds = "0"+seconds;
    }
    if(hours<10)
    {
        hours = "0"+hours;
    }
    $("#time").html(hours + ":" + minutes + ":" + seconds);

    if(hours>11)
    {
        $("#time").append(" PM");
    }
    else
    {
        $("#time").append(" AM");
    }
    setTimeout(updateClock, 1000);
}
\$\endgroup\$

1 Answer 1

Reset to default
2
\$\begingroup\$

First of all, your code's just fine as-is. Still, there are improvements that could be made.

  1. This isn't about the JavaScript, but the HTML: You're loading a lot of stuff twice for some reason. Both the google fonts and jQuery are loaded both as // and as http://. Only use the latter option: http://.
    When the browser sees a URL starting with //, it'll simply reuse the protocol of the page. This is clever on a site that might be http or https since the browser will use the appropriate protocol. But it won't work if you're viewing the site directly on you own computer using a file:// url, because the browser will try to find file://fonts.googleapis.com/... etc.. So just use the http: urls - they'll work fine everywhere.

  2. You've split the javascript into 2 files, but they're very dependent on each other. So if one file gets loaded, but the other one doesn't, it won't work. I'd put it all in a single file - especially as mainScript.js contains very little code.

  3. Since you're using jQuery, use it. In mainScript.js you're using addEventListener which won't work in all browsers - instead use jQuery's .on() which is cross-browser.
    Better yet, use jQuery's own shortcut by simply passing a function to $():

    $(function () {
  // code to call when the window has loaded
});

  4. Don't pollute the global scope. If you keep everything in one file, nothing needs to be in the global scope to be accessible. Instead use the code above, and put everything into that function. It'll all be nicely kept out of the global scope.

  5. Don't use the "curly-brace on newline" style in JavaScript. In other languages it's OK, but JavaScript will sometimes treat a linebreak as an end-of-line and insert a semi-colon that it thinks you forgot.
    This is obviously weird, but that's the way it is, unfortunately. See Douglas Crockford's page for more.

  6. Just a quick tip: Instead of checking for x < 10 to see if you need a leading zero, just add 2 zeros in front, and take the last 2 characters/digits of the resulting string:

    function updateClock() {
    var currentTime = new Date(),
        hours = currentTime.getHours(),
        minutes = currentTime.getMinutes(),
        seconds = currentTime.getSeconds(),
        suffix = hours > 11 ? "PM" : "AM";

    // internal helper function
    function pad(value) {
      return ("00" + value).slice(-2); // take the last 2 digits
    }

    $("#time").text( pad(hours) + ":" + pad(minutes) + ":" + pad(seconds) + " " + suffix );

    setTimeout(updateClock, 1000);
}

Technically, you should also subtract 11 from the hours, if you're using the AM/PM notation and it's PM. Saying that the time is "23:14:20 PM" is sort of redundant - of course it's PM.

\$\endgroup\$
8
  • \$\begingroup\$ wow! those were some great tips. Thanks very much. But one thing that I was really doubtful about was the way I have a function called drawLine with so many arguments. I am going to have more functions like this like drawCircle, drawRect etc. It might become a bit too much code, too many arguments. Is that okay or would it better to do something else? \$\endgroup\$
    – Joe Slater
    Commented Mar 19, 2013 at 20:27
  • 1
    \$\begingroup\$ @AnkurSharma You can have as many arguments as you want, but it can sometimes look and feel like too much. But for canvas-related stuff, it's difficult to avoid. The alternatives are to either just use the canvas drawing functions directly, or make ever-more specific functions, e.g. a drawCenteredCricle() which doesn't need position arguments. It can then call a more generic drawCircle function, which then calls the canvas functions, and so on. Personally, I'd probably set the colors using the native canvas API, and use the custom functions to just draw the shapes. \$\endgroup\$
    – Flambino
    Commented Mar 19, 2013 at 21:06
  • \$\begingroup\$ Ok thanks very much. But I was thinking about the global scope that you mentioned. Are you saying that if I put a variable or function outside everything it is global, even if i declared it with a var keyword? \$\endgroup\$
    – Joe Slater
    Commented Mar 19, 2013 at 21:25
  • 1
    \$\begingroup\$ @AnkurSharma Yes, they'll be global. The var keyword basically means "declare a variable in the current scope", so if your current scope is the global scope (which it is, if you're "outside everything"), then that variable will naturally be global. The most common trick is to use an immediately-invoked anonymous function to create a scope: (function () { ... your code }());. Since it's anonymous, it won't itself end up in global scope. In your case, though, $(function () {...}); will do the same trick; an anonymous function invoked on load instead of immediately. \$\endgroup\$
    – Flambino
    Commented Mar 19, 2013 at 21:34
  • 1
    \$\begingroup\$ @AnkurSharma to have stuff available globally, it's best practice to create a namespace for your code, and only let that be global. Start each file with window.myNamespace || (window.myNamespace = {}); to create the myNamespace object if it's missing. Then add functions to that: myNamespace.drawSomething = function () { ... }; or myNamespace.someVar = 42;. Then you can access it anywhere "through" the myNamespace namespace. See this answer for more (it's about CoffeeScript, but the strategy is the same) \$\endgroup\$
    – Flambino
    Commented Mar 19, 2013 at 21:41

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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