1
\$\begingroup\$

As this community is full of experienced JavaScript developers, and I'm a newbie in JavaScript, I would like if some of you could look over my code for a website I'm building (personal portfolio) and give some insight on how I can improve it, maybe make it more readable and clean, things I may be doing wrong or I missed.

And here's the JavaScript (excluding plugins and other JavaScript libraries):

$(document).ready(function() {

var default_cluster_options = {
    environment             : "Development",
    local_storage_key       : "Cluster",
    plugin_navigation_class : ".navigation",
    plugin_wrapper_id       : "content-wrapper",
    headings                : ['.heading-first h1', '.heading-second h1'],
    input_types             : ['input', 'textarea'],
    info_iqns_class         : ".iqn",
    preview_iqn_class       : ".preview",
    limits                  : [ { min: 1224, items: 8 }, { min: 954, items: 6 }, { min: 624, items: 4 }, { min: 0, items: 2 } ],
    shop_local_storage_key  : "Shop",
};

var default_plugin_options = {
    containerID : "",
    first       : false,
    previous    : false,
    next        : false,
    last        : false,
    startPage   : 1,
    perPage     : 1,
    midRange    : 6,
    startRange  : 1,
    endRange    : 1,
    keyBrowse   : false,
    scrollBrowse: false,
    pause       : 0,
    clickStop   : true,
    delay       : 50,
    direction   : "auto",
    animation   : "fadeIn",
    links       : "title",
    fallback    : 1000,
    minHeight   : true,
    callback    : function(pages, items) {}
};

var Cluster = function(cluster_options, plugin_options) {

    var self = this;

    this.options = $.extend({}, default_cluster_options, cluster_options);

    this.plugin_options = $.extend({}, default_plugin_options, plugin_options);

    this.environment = this.options.environment;

    this.data_key = this.options.local_storage_key;

    this.shop_data_key = this.options.shop_local_storage_key;

    this.plugin_navigation_class = this.options.plugin_navigation_class;

    this.plugin_wrapper_id = this.options.plugin_wrapper_id;

    this.headings = this.options.headings;

    this.input_types = this.options.input_types;

    this.viewport = $(window);

    this.body = $('body');

    this.viewport_width = this.viewport.width();

    this.viewport_height = this.viewport.height();

    this.info_iqns_class = this.body.find(this.options.info_iqns_class);

    this.preview_iqn_class = this.body.find(this.options.preview_iqn_class);

    this.limits = this.options.limits;

    this.current_shop_page = this.options.current_shop_page;

    this.total_shop_pages = this.options.total_shop_pages;

    this.initiate_cluster(self.plugin_navigation_class, {
        containerID : self.plugin_wrapper_id,
        startPage : +(self.get_local_storage_data(self.data_key) || 1),
        callback : function(pages){
            self.set_local_storage_data(self.data_key, pages.current);
        }
    });

    this.inititate_shop();

    this.initiate_shop_touch_events();

};

Cluster.prototype.set_environment = function() {
    if(this.environment == "Development") {
        less.env = "development";
        less.watch();
    }
};

Cluster.prototype.set_local_storage_data = function(data_key, data_val) {
    return localStorage.setItem(data_key, data_val);
};

Cluster.prototype.get_local_storage_data = function(data_key) {
    return localStorage.getItem(data_key);
};

Cluster.prototype.initiate_scalable_text = function() {
    for(var i in this.headings) {
        $(this.headings[i]).fitText(1.6);
    }
};

Cluster.prototype.initiate_placeholder_support = function() {
    for(var i in this.input_types) {
        $(this.input_types[i]).placeholder();
    }
};

Cluster.prototype.initiate_iqn_selected_class = function() {
    if(this.viewport_width < 980) {
        $(this.info_iqns_class).each(function(index, element) {
            var iqn = $(element).parent();
            $(iqn).on('click', function() {
                if($(iqn).hasClass('selected')) {
                    $(iqn).removeClass('selected');
                } else {
                    $(iqn).addClass('selected');
                }
            });
        });
    }
};

Cluster.prototype.initiate_preview_action = function() {
    $(this.preview_iqn_class).each(function(index, element) {
        var data = $(element).attr('data-image-link');
        $(element).on('click', function(ev) {
            $.lightbox(data, {
                'modal'         : true,
                'autoresize'    : true
            });
            ev.preventDefault();
        });
    });
};

Cluster.prototype.initiate_plugin = function(plugin_navigation, plugin_options) {
    var options = $.extend({}, this.plugin_options, plugin_options);
    return $(plugin_navigation).jPages(options);
};

Cluster.prototype.initiate_shop_touch_events = function() {
    var self = this;
    return $("#shop-items-wrapper").hammer({prevent_default: true, drag_min_distance: Math.round(this.viewport_width * 0.1)}).bind("drag", function(ev) {
        var data = JSON.parse(self.get_local_storage_data(self.shop_data_key));
        if (ev.direction == "left") {
            var next_page = parseInt(data.current_page + 1);
            if(next_page > 0 && next_page <= data.total_pages) {
                $(".shop-items-navigation").jPages(next_page);
            }
        }
        if(ev.direction == "right") {
            var prev_page = parseInt(data.current_page - 1);
            if(prev_page > 0 && prev_page <= data.total_pages) {
                $(".shop-items-navigation").jPages(prev_page);
            }
        }
    });
}

Cluster.prototype.inititate_shop = function() {
    var self = this;
    for(var i = 0; i < this.limits.length; i++) {
        if(this.viewport_width >= this.limits[i].min) {
            this.initiate_plugin('.shop-items-navigation', {
                containerID : "shop-items-wrapper",
                perPage     : self.limits[i].items,
                midRange    : 8,
                animation   : "fadeIn",
                links       : "blank",
                keyBrowse   : true,
                callback    : function(pages) {
                    var data = {
                        current_page : pages.current,
                        total_pages  : pages.count
                    }
                    self.set_local_storage_data(self.shop_data_key, JSON.stringify(data));
                }
            });
            return false;
        }
    }
};

Cluster.prototype.initiate_cluster = function(plugin_navigation, plugin_options) {
    this.set_environment();
    this.initiate_scalable_text();
    this.initiate_placeholder_support();
    this.initiate_iqn_selected_class();
    this.initiate_preview_action();
    this.initiate_plugin(plugin_navigation, plugin_options);
};

var cluster = new Cluster();

});
\$\endgroup\$
  • 1
    \$\begingroup\$ For a "newbie in JavaScript" your web page uses JavaScript awfully heavily. Most importantly it's completely broken with JavaScript disabled. If it were a "web application", I could understand that, but considering that it's a trivial "content site" and that I can't see any functionality that actually requires JS, I personally find that unacceptable. :-( \$\endgroup\$ – RoToRa May 18 '12 at 15:26
  • 1
    \$\begingroup\$ Regarding your script: For a code review it would help to have a short explanation what it is supposed to do. \$\endgroup\$ – RoToRa May 18 '12 at 15:29
  • \$\begingroup\$ Perhaps your opinion it's a bit wrong, how can you say JS isn't necessary ? Do you know of any CSS techniques that could do what the jPages plugin does ? Or how about detecting CSS3 features with Modernizr ? And other fallbacks in JS that some browsers don't support ? And as a simple notice, what browser does not support JS now, 1% of the Earth's population ? \$\endgroup\$ – Roland May 18 '12 at 15:30
  • \$\begingroup\$ @RoToRa I think it's pretty obvious what it does for someone who knows JS. Or should I describe what every plugin I use does ? I'm not sure I follow what you're suggesting \$\endgroup\$ – Roland May 18 '12 at 15:31
  • 1
    \$\begingroup\$ I'm not saying you shouldn't use any JS at all. I'm saying it's possible to have the site usable without JS and the functionality I currently see with much less JS. And gbot doesn't need to index the JS, but the content. Currently if a search engine sends someone to your site, they will get the "homepage" but most likely not the content they were looking for (for example the "products"). (NB the site seems to be currently broken. LESS seems to be missing. BTW the JS implementation of LESS should only be used for development not the live site). \$\endgroup\$ – RoToRa May 18 '12 at 22:04
1
+50
\$\begingroup\$

In the Cluster.prototype.initiate_shop_touch_events function, the only difference between "left" and "right" is the number that gets added to the current page. You can reduce duplication by pulling that out into a variable:

        var step = (ev.direction == "left") ? 1 : -1;
        var new_page = parseInt(data.current_page + step);
        if(new_page > 0 && new_page <= data.total_pages) {
            $(".shop-items-navigation").jPages(new_page);
        }
\$\endgroup\$
  • \$\begingroup\$ I don't get the point here, what if the direction is right ? It actually needs the ev.direction == "right" to test which direction the drag event has been fired \$\endgroup\$ – Roland May 19 '12 at 18:24
  • \$\begingroup\$ I was assuming that direction is only one of two values: "left" or "right". You can use whatever logic is appropriate to set the value of step, but the key point is to reduce unnecessary duplication. \$\endgroup\$ – Cheran Shunmugavel May 19 '12 at 22:35
  • \$\begingroup\$ Yes, I get the logic, only that the drag event can occur to whatever direction you drag your finger along the touch screen :) I'll try this one anyway :) \$\endgroup\$ – Roland May 20 '12 at 10:31
2
\$\begingroup\$

Why is Cluster a function? You only instantiate it once, and you don't export anything (as far as I have seen). Use a simple object instead.

If you want to export the Cluster, you should not do it onDOMReady, but as soon as your code executes.

var Cluster = (function(){
    // private static vars like default_options

    function Cluster() { ... }

    Cluster.prototype = ...

    return Cluster;
})();
// after that, you might instantiate one
jQuery(function(){
    var cluster = new Cluster;
});

Also, it might be a good idea to put the onDOMready into the .inititate_XY() methods, if they need it. They seem to already use a callback, so it should be no problem - I'm not sure whether that matches your requirements.

\$\endgroup\$
  • \$\begingroup\$ I don't think that using onDOMReady inside the methods might be a good idea, I already have the entire prototype function wrapped inside $(document).ready(function() {}); so there's no need to reuse it, just my opinion. Is it wrong if I'm using it as a function ? What impact will your method have on the website ? Oh, and for now it's only a piece of the entire project, there much more I need to do in fact, but I want to make sure I'm on the right track. \$\endgroup\$ – Roland May 18 '12 at 14:47
1
\$\begingroup\$

For places where you have an array of selectors (such as this.headings), you don't need to loop through each element of the array (never mind that for...in with arrays is a bad idea). Instead, you can use jQuery's multiple selector to create one selector. This should perform slightly better since there will be less function calls to the plugins.

Example

Cluster.prototype.initiate_scalable_text = function() {
    var selector = this.headings.join(",");
    $(selector).fitText(1.6);
};
\$\endgroup\$
  • \$\begingroup\$ Indeed, this might perform a bit faster. But why looping through an array like I do is not safe ? \$\endgroup\$ – Roland May 19 '12 at 18:24
  • \$\begingroup\$ @Roland, did you read the linked SO question? \$\endgroup\$ – Cheran Shunmugavel May 19 '12 at 22:36
  • \$\begingroup\$ I have now :) Seems like a good reason why not, even though in my case I don't think that something like that will occur. Anyway, using jQuery selectors might perform faster :) \$\endgroup\$ – Roland May 20 '12 at 10:27

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.