2
\$\begingroup\$

This is my first 'large' JS project and I was hoping someone could have a quick look and suggest better coding practices, how it could be cleaner etc. It revolves around a one page web-site and contains some AJAX.

    $(document).ready(function () {
        var curpoints;

        //int globals
        var sectionloaded = 'about';
        var History = window.History;
        var vph = window.innerHeight;
        var vpw = window.innerWidth;


        //setup size
        $('#master').css('height', vph * 2);
        $('section').css('height', vph);
        $('#brief').addClass('inview');

        //check url
        var urlquery = "<?php echo $query; ?>";
        if (urlquery == "portfolio") {
            navi('portfolio', 200);

        } else if (urlquery == "resume") {
            navi('resume', 200);
        };

        //prevent nav a s 
        $('header a').click(function (e) {
            e.preventDefault();
        });
        $('article a').click(function (e) {
            e.preventDefault();
        });

        //resize
        $(window).resize(function () {

            vph = window.innerHeight;
            $('section').css('height', vph);
            $('body').css('height', vph);
            $('#master').css('height', vph * 2);
            if ($('#skills').hasClass('inview')) {
                $('#master').css('margin-top', -vph);
            };


        });


        //article container stuff
        $('#entrycontainer article').click(function () {
            curid = $(this).data('url');
            navi('portfolio', 1000);

        })

        $('body').mousedown(function (e) {
            if (e.button == 1) return false
        });


        //NAV SECTION
        /////////////////
        //Buttons
        $('#port').click(function () {
            navi('portfolio', 1000);

        });

        $('#resu').click(function () {
            navi('resume', 1000);
        });

        $('#firstli').click(function () {
            navi('about', 1000);
        });
        $('#conatctbut').click(function () {
            navi('contact', 1000);
        });

        /////////
        //Back/Forward buttons
        /////////



        window.addEventListener("popstate", function (e) {


            curstate = window.location.href;

            curstatereg = curstate.substring(7);
            var splituri = curstatereg.split('/');
            var reg = /^mydomain\.com\/portfolio\/.*/;
            if (reg.test(curstatereg)) {

                $.get('../ajax1.php?page=' + splituri[2], function (data) {
                    curid = data;
                    navi('portfolio', 200);


                });
            };

            switch (curstate) {

            case 'http://mydomain.com/':

                navi('about', 200);
                break;



            case 'http://mydomain.com/resume':
                navi('resume', 200);
                break;

            case 'http://mydomain.com/contact':
                navi('contact', 200);
                break;
            };

        });





        function navi(pagetoload, speed) {

            if (pagetoload == 'about') {
                if (sectionloaded == 'about') {
                    return
                } else {
                    $('#master').stop();
                    $('#master').animate({
                        marginTop: 0
                    }, 1000);
                    sectionloaded = pagetoload;
                    History.pushState(null, pagetoload, '/');
                }
            } else {


                if (sectionloaded == pagetoload) {
                    $.getJSON('../ajax.php?id=' + curid, function (data) {

                        if (data.tags == null) {
                            return
                        } else {
                            tags = data.tags.split(',');
                            taglen = tags.length;
                        };

                        $('#tags').children('li').remove();
                        $("#portcontainer").fadeOut(300, function () {
                            $('#textcontainer h1').html(data.title);
                            $('#textcontainer p').html(data.body);
                            $('#textcontainer a').attr('target', '_blank');
                            $('#textcontainer a').attr('href', '../live/' + data.points);
                            $('#portbox img').attr('src', '../' + data.img);
                            $('#tags').children('li').remove();
                            for (i = 0; i < taglen; i++) {
                                $('#tags').append("<li>" + tags[i] + "</li>");
                            }

                        });
                        $("#portcontainer").fadeIn(300);
                        History.pushState(null, data.title, '/portfolio/' + data.points);
                        curid = data.id;
                        curpoints = data.points;

                    });
                } else if (sectionloaded == 'about') {
                    $.get('../content.php?which=' + pagetoload, function (data) {
                        $('#container').html(data);

                        if (pagetoload == 'portfolio') {
                            intport();
                        } else {
                            History.pushState(null, data.title, '/' + pagetoload);
                        };

                        $('#master').stop();
                        $('#master').animate({
                            marginTop: -vph
                        }, speed);
                        var link = $("<link>");
                        $('#dynamic').attr('href', '../' + pagetoload + '.css');
                        sectionloaded = pagetoload;



                    })
                } else {
                    $.get('../content.php?which=' + pagetoload, function (data) {
                        $('#container').fadeOut(500, function () {
                            $('#container').html(data);
                            if (pagetoload == 'portfolio') {
                                intport();
                            } else {
                                History.pushState(null, data.title, '/' + pagetoload);
                            };
                            $('#dynamic').attr('href', '../' + pagetoload + '.css');
                        });

                        sectionloaded = pagetoload;

                        $('#container').fadeIn();


                    });

                };
            };
        };


        //
        ///////////////
        //AJAX STUFF FOR PORTFOLIO 
        //////////////
        <?
        php
        if (isset($portfolio)) {
            $data1 = mysql_query("SELECT ind FROM project WHERE points ='".$portfolio."'")
            or die(mysql_error());
            $info1 = mysql_fetch_array($data1);
            echo 'var curid = '.$info1[ind].';';

            echo "navi('portfolio',300);";
        } else {
            echo 'var curid = 0;';
        } ?>
        function intport() {



            var rows;
            $.getJSON('../ajax.php?id=' + curid, function (data) {

                tags = data.tags.split(',');
                taglen = tags.length;
                rows = data.rows;
                $('#tags').children('li').remove();

                $('#textcontainer h1').html(data.title);
                $('#textcontainer a').attr('href', '../live/' + data.points);
                $('#textcontainer a').attr('target', '_blank');
                $('#textcontainer p').html(data.body);
                $('#portbox img').attr('src', '../' + data.img);
                $('#tags').children('li').remove();
                for (i = 0; i < taglen; i++) {
                    $('#tags').append("<li>" + tags[i] + "</li>");
                }
                History.pushState(null, data.title, '/portfolio/' + data.points);
                curpoints = data.points;


                curid = data.id;
            });




            $('#buttonr').click(function () {


                if (curid == rows - 1) {

                    curid = 0;
                    ajaxproj();

                } else {
                    curid++;
                    ajaxproj();
                }
            });
            $('#buttonl').click(function () {


                if (curid == 0) {

                    curid = rows - 1;
                    ajaxproj();

                } else {
                    curid--;

                    ajaxproj();
                }
            });

            function ajaxproj() {
                $.getJSON('../ajax.php?id=' + curid, function (data) {

                    if (data.tags == null) {
                        return
                    } else {
                        tags = data.tags.split(',');
                        taglen = tags.length;
                    };

                    $('#tags').children('li').remove();
                    $("#portcontainer").fadeOut(300, function () {
                        $('#textcontainer h1').html(data.title);
                        $('#textcontainer p').html(data.body);
                        $('#textcontainer a').attr('target', '_blank');
                        $('#textcontainer a').attr('href', '../live/' + data.points);
                        $('#portbox img').attr('src', '../' + data.img);
                        $('#tags').children('li').remove();
                        for (i = 0; i < taglen; i++) {
                            $('#tags').append("<li>" + tags[i] + "</li>");
                        }

                    });
                    $("#portcontainer").fadeIn(300);
                    History.pushState(null, data.title, '/portfolio/' + data.points);
                    curid = data.id;
                    curpoints = data.points;

                });
            }


        };



    });
\$\endgroup\$
7
\$\begingroup\$

Giving your code a quick look does not make me understand a single line. Will you understand it if you go away for two weeks?

Giving it a second look, I notice you've added some short comments describing each block. That tells me each block is an excellent candidate for its own function.

Notice in the following example how the function given to $(document).ready() now reads like a small story instead of a lot of detailed jquery instructions.

(function() {

    var sectionLoaded;
    var history;
    var windowWidth, windowHeight;

    $(document).ready(function() {
        initializeGlobals();
        setupSize();
        checkUrl();
        // ...
    });

    function initializeGlobals() {
        sectionloaded = 'about';  
        history = window.history;  
        windowHeight = window.innerHeight;  
        windowWidth = window.innerWidth;  
    }

    function setupSize() {
        $('#master').css('height', vph * 2); 
        $('section').css('height', vph); 
        $('#brief').addClass('inview'); 
    }

    function checkUrl() {
        var urlquery = "<?php echo $query; ?>"; 
        if (urlquery == "portfolio") { 
            navi('portfolio', 200); 
        } else if (urlquery == "resume") { 
            navi('resume', 200); 
    };

})();

Secondly, check url does not tell me anything. Why check the url? And inside that, some function called navi is called. What's navi? I'm guessing navigate, but then why isn't it called navigate?

Same goes for vph and vpw - what does vp stand for? Why not call them windowWidth and windowHeight?

I'm also quite unsure what buttonr and button1 does.

Guess you get my point by now. :) Function names should scream out what they do. If they do, you don't need comments.

You should also look into closures to have your globals scoped for this functionality instead of "polluting" the global namespace.

\$\endgroup\$
  • \$\begingroup\$ Thanks Lars , some really sensible suggestions there . I shall implement them . \$\endgroup\$ – Frank Astin Feb 14 '12 at 10:36
  • \$\begingroup\$ :) feel free to vote up or tag as answer. \$\endgroup\$ – Lars-Erik Feb 14 '12 at 11:34
  • \$\begingroup\$ Everything works great apart from the initializeGlobals function. If I try to invoke the function inside or out of the document ready it doesn't work. I Have to declare them outside the doc ready . \$\endgroup\$ – Frank Astin Feb 14 '12 at 14:57
  • \$\begingroup\$ @Lars-Erik +1 excellent answer! \$\endgroup\$ – ComputerSaysNo Feb 14 '12 at 16:24
  • \$\begingroup\$ @FrankAstin initializeGlobals() must be outside of $(function(){ }) part, otherwise it cannot be accessed outside of the document ready scope \$\endgroup\$ – ComputerSaysNo Feb 14 '12 at 16:24

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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