3
\$\begingroup\$

As a very novice developer, I'm reading more about OOP and structuring JavaScript in a sensible way and have been using the following format for a little while now. It's difficult to know the best way to do things because most examples I see online are for much larger websites than I currently build. More like applications than websites, spanning multiple files and thousands of lines of code, where as mine is just in a single JS file spanning around 1k lines at most. I'm also using jQuery on each project and most things don't use a library like this, or they're based around Angular, etc.

jQuery(document).ready(function($){

    Site.init();

});


var Site = (function($) {


    // ================ !CACHING COMMON VARS

    // Aliased Variables
    var cfg, throttle, helper, loader; 

    // DOM caching
    var $win = $(window),
        $doc = $(document);

    // Globals
    var w = {
        width: $win.width(),
        height: $win.height(),
        scroll:  $doc.scrollTop()
    }


    return {


        // ================ !SCOPE SETTINGS

        settings: {
            speed_fast: 300,
            speed_slow: 600,
            easing: [0.55, 0, 0.1, 1] // SwiftOut (Google Material Design)
        },




        // ================ !SITE

        init: function() {

            // Aliases
            cfg = this.settings;
            throttle = this.handlers.throttle;
            helper = this.helpers;
            loader = this.handlers.loader;


            // ================ !SITEWIDE FUNCTIONS

            // Form validation & ajax
            this.formProcess();



            // ================ !SPECIFIC FUNCTIONS

            // Share Popups
            helper.check('.js-social-share', this.sharePopup);
        },




        // ================ !EVENT-HANDLERS

        handlers: {

            // Throttle resize and scroll events
            throttle: function(handler, func) {
                var eventHappend = false,
                    throttleOps = {};

                throttleOps[handler] = function(){
                    eventHappend = true;
                }

                $win.on(throttleOps);

                setInterval(function() {
                    if ( eventHappend ) {

                        eventHappend = false;

                        if (handler === 'resize') {

                            window_width = $win.width();
                            window_height = $win.height();
                        }

                        if (handler === 'scroll') {

                            scroll_top = $doc.scrollTop();
                        }

                        func();
                    }
                }, 250);
            },



            loader: function(func) {
                $win.on('load', func);
            }

        }, // End Handlers



        // ================ !HELPER/UTILITY FUNCTIONS

        helpers: {

            // Get a random Num between min and max
            getRandom: function(min, max) {
                return Math.floor(Math.random() * (max - min + 1)) + min;
            },

            // Check if element exists and run function
            check: function(el, func) {
                if ($(el).length) {
                    func();
                }
            }

        }, // End Helpers





        // ================ !SITEWIDE FUNCTIONS

        // Open Share popups in new window
        sharePopup: function() {
            $('.js-social-share a').on('click', function(){

                if ($(this).attr('href').indexOf('http') === 0) {
                    var newwindow = window.open($(this).attr('href'), '', 'height=450, width=700');

                    if (window.focus) {
                        newwindow.focus();
                    }

                    return false;

                }
            });
        }, // End sharePopup()






        // ================ !SPECIFIC FUNCTIONS


        // Process Forms.
        formProcess: function() {
            var $forms = $('.js-process-form');


            // Make sure forms are valid (using jQuery Validate)
            $forms.each(function(){
                $(this).validate({
                    ignore: '',
                    rules: {
                        upload: {
                            required: true
                        }
                    },
                    onfocusout: function(element) {
                        this.element(element);
                    },
                    onkeyup: false
                });
            });



            // On submit do Ajax post
            $forms.on('submit', function(e){
                e.preventDefault();

                var $this = $(this);

                if ($this.valid()) {

                    // Uses site object from Worpress
                    $.ajax({
                        url: WP.base_dir+"/wp-admin/admin-ajax.php",
                        type:'POST',
                        data: $.extend({
                            action:     'contact_email'
                        }, $this.serializeObject()),
                        dataType: 'json',
                        success: function(data){
                            //Access the returned JSON
                            $this.html('<h1>' + data.message + '</h1>');
                        }
                    });
                }
            });
        }, // End formProcess()



    }; // End return Site

})(jQuery);

Main things that are bothering me are:

  • Are things enclosed correctly? (so as to not conflict with anything another dev might add in to the domready)
  • Are the event handlers/checkers even efficient?
  • Am I doing anything glaringly wrong?

There are a few things that are included elsewhere, so don't worry about something that is referenced but not in the script. I've included a couple of example functions in there as I would normally use them.

\$\endgroup\$
1
\$\begingroup\$

First, I would question why the Site object has so many properties in its API. It looks to me that Site.init is the only method that needs to be public and that all of the other methods/objects (Site.settings, Site.handlers, Site.helpers, Site.sharePopup, and Site.formProcess) are all intended to be used internally to the Site object implementation and do not need to be "public".

With that said, if Site exists only as an interface to a single method, why not simply replace the Site object with a function?

var initSite = function ($) {
    var settings, handlers, helpers, sharePopup, formProcess;
    /* Site initialization implementation goes here. */
};

And our ready handler would become:

jQuery(initSite);

On another note, I do not like the way that your selector strings are scattered around your code and are hard-coded with the feature implementations (sharePopup and processForm). I would recommend moving the selectors into a single configuration object and having your feature initializers take a selector or jQuery object as a parameter.

var initForms = function ($forms) {
    $forms.on('submit', function (e) {
        /* Do AJAX handling here. */
        e.preventDefault();
    });
};

var initSharePopup = function ($anchor) {
    $anchor.on('click', function (e) {
        /* Do Popup stuff here. */
        e.preventDefault();
    });
};

var initializers = {
    '.js-process-form': initForms,
    '.js-social-share a': initSharePopup
};

Any additional features we want to include in our pages can be built with init method similar to the ones above and implementing the same interface. Our site initialization will now be responsible for iterating through our initializers object and calling the init methods for selectors it finds in the document (essentially doing the work of your helper.check method).

$.each(initializers, function (selector, initializer) {
    var $els = $(selector);
    if ($els.length > 0) {
        initializer($els);
    }
});

This way, when we want to add features/modules to our page, we need only to add a selector, initializer pair to our initializers object, and our feature will get initialized on pages only when the corresponding selector is found.

\$\endgroup\$
  • \$\begingroup\$ Thanks 76484, I have a couple of questions regarding your reasoning though: Is it more performant as a function rather than an object? The object syntax seems more readable to me and there have been a couple of times where I've used Site functions outside of init. And what is the difference between your initialiser and the way I am doing it? Clearly it's different but is there performance gains? And a question about those initialisers, is it better to check for an element before running the function or does the function get stored in memory anyway? Thanks for the feedback! :) \$\endgroup\$ – evu Nov 2 '15 at 8:55
  • \$\begingroup\$ I can't imagine there would be any noticeable performance gain either way. I am aiming for readability and maintainability with my solution. I find that your Site object is just too big and does too much for me to wrap my head around. I do think there are gains in readability and reusability in my version because each feature initializer is its own module. You could use the initializer method on all of your projects and you would need only to update the initializers configuration object. \$\endgroup\$ – 76484 Nov 3 '15 at 1:07
  • \$\begingroup\$ The reason I check for an element before invoking an initializer with it is simply because it would be pointless to call the initializer with no element(s) to affect. \$\endgroup\$ – 76484 Nov 3 '15 at 1:07

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.