2
\$\begingroup\$

The code I have now is currently working, but I'm looking for ways to optimize it, and really, to just get any feedback.

I'm implementing this code through Ensighten (a tag management platform) as a custom javascript tag. Ensighten works by injecting tags either sync or async scripts on to a page. I've set this tag to fire after the DOM is ready.

What it does is creates a namespace, and then sets a function in that namespace to main (see main.js). The main function takes one argument, an object (see lookupTable.js), in which it takes that object, loops through the object, and if the object contains the correct key, it sets that key's value equal to campaignCode.

Next, the function looks to see if the campaignCode is not falsey, it then looks at the path (window.location.path) to see if it's either a courses or a topics page. If it's a topics page it fires the analytics code. If it's a courses page, it listens for a message from an iframe that a video was clicked. If the message from that iframe equals play button clicked then it fires the other analytics code.

The main function is called from the the lookupTable.js. Like so:

window.analytics.campaign.main(window.analytics.campaign.lookupTable);

Questions:

  • Should I be checking if the main function's argument is an object?
  • Is there a way I wouldn't have to check if the campaignCode is falsy? Could I check it in the if statement of the for loop?
  • Am I declaring the analyticsTrack function in the correct way?

main.js

/*jslint browser:true*/
/*globals Bootstrapper*/

window.analytics = window.analytics || {};
window.analytics.campaign = window.analytics.campaign || {};

window.analytics.campaign.main = function (lookupTable) {
    var path = window.location.pathname,
        campaignCode = '',
        prop;

    function analyticsTrack (id) {
        return Bootstrapper.insertScript('//www.example.com/click/services/Tracking/Click.ashx?CR_EAC=' + id);
    }

    for (prop in lookupTable) {
        if (lookupTable.hasOwnProperty(prop) && path === prop) {
            campaignCode = lookupTable[prop];
        }
    }

    // check campaignCode
    if (campaignCode) {

        // if it's a topic page, fire a RIO tag onload
        if (path.indexOf('topics') > -1) {
            analyticsTrack(campaignCode);
        }

        // if it's a courses page
        if (path.indexOf('courses') > -1) {

            // listen for a message from the iframe
            $(window).on('message', function (e) {

                // check the message from the iframe, fire a RIO tag
                if (e.originalEvent.data === 'play button clicked') {
                    analyticsTrack(campaignCode);
                }
            });
        }
    }
};

lookupTable.js

window.analytics = window.analytics || {};
window.analytics.campaign = window.analytics.campaign || {};

var campaign = window.analytics.campaign;

campaign.lookupTable = {
    '/topics/123456':    '300257956',
    '/courses/234567':   '300257957',
    '/courses/34567':    '300257958',
    '/courses/15677789': '300257959',
    '/topics/123457':    '300257970'
};

campaign.main(campaign.lookupTable);
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$
  • Should I be checking if the main function's argument is an object?

No, document it as it accepts objects, the params are straight forward no need to check them for the consumer

  • Is there a way I wouldn't have to check if the campaignCode is falsy? Could I check it in the if statement of the for loop?

No need to loop just access the property directly

campaignCode = lookupTable[prop];

  • Am I declaring the analyticsTrack function in the correct way?

Yes and no, every time you invoke the main function you are allocating the method. There are no closures around it so it doesn't need to be done. Just move it outside of the main method but wrap everything in a self invoking method (if you are not already with some other tool) so it won't be global.

If I had to rewrite everything here is how I would do it. Instead of having a bunch of if path contains x then do something with the campainCode statements I just put it in an object. It might be overkill but it is a good pattern to use in other places. Say you want to handle books instead of adding another if indexof .. just add books to the pathsHandlers object.

(function() {

    function analyticsTrack(id) {
        return Bootstrapper.insertScript('//www.example.com/click/services/Tracking/Click.ashx?CR_EAC=' + id);
    }

    var pathHandlers = {
        topics: function(campaignCode) {
            analyticsTrack(campaignCode)
        },
        courses: function(campaignCode) {
            $(window).on('message', function(e) {

                // check the message from the iframe, fire a RIO tag
                if (e.originalEvent.data === 'play button clicked') {
                    analyticsTrack(campaignCode);
                }
            });
        }
    }

    window.analytics.campaign.main = function(lookupTable) {
        var path = window.location.pathname,
            campaignCode = lookupTable[path],
            handlerKey;

        if (campaignCode) {
            for (handlerKey in pathHandlers) {
                if (pathHandlers.hasOwnProperty(handlerKey) && path.indexOf(handlerKey) > -1) {
                    pathHandlers[handlerKey](campaignCode);
                }
            }
        }
    };

})();

--edit to answer comment

Your function was defined in the main function but it didn't need to be because it wasn't using any locally scoped variables in main (no variables being accessed in the function via closure).

If the method was for example :

window.analytics.campaign.main = function (lookupTable) {
    var path = window.location.pathname,
        campaignCode = '',
        prop;

    function analyticsTrack (id) {
        console.log('current path is ' + path)
        return Bootstrapper.insertScript('//www.example.com/click/services/Tracking/Click.ashx?CR_EAC=' + id);
    }

The method couldn't be moved outside of main because it is referencing the path variable within a closure.

\$\endgroup\$
3
  • \$\begingroup\$ thanks a lot for the review. Like the idea of building an object for the paths rather than checking indexOf each time I add a new - there will be new ones too :) Ensighten wraps every custom javascript tag in a IIFE. Could you explain what you meant by 'There are no closures around it so it doesn't need to be done'? Again, really appreciate you taking a look! Go Pack Go - I'm from Wisconsin originally and recognized D. Adams. \$\endgroup\$
    – Blexy
    Commented Feb 24, 2015 at 16:52
  • \$\begingroup\$ @Blexy I updated my answer to answer your question. I am a big Packers fan but that is not Devonte haha, just an obscure Packer that has the same name as me :) \$\endgroup\$
    – pllee
    Commented Feb 24, 2015 at 18:10
  • \$\begingroup\$ ah, got it. Thanks for the update. Not Devonte? I could have sworn it was! Anyways, looking forward to next season! Cheers! \$\endgroup\$
    – Blexy
    Commented Feb 24, 2015 at 18:20

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.