3
\$\begingroup\$

I'm keen to get some feedback on how I have utilized the Revealing Module pattern here.

This code is part of a web application that is used by the public. The purpose of this code is to encapsulate all logic concerned with sending events to Google Analytics within a module...called GAU, which is short for googleAnalyticsUtils.js.

The code smells I detect are:

  1. The way I'm tracking if the 'Submit' button has been clicked within the GAU module

  2. I'm handling the click event for the #btn-another-report button twice...once in within reportAnOutage.js and again in the GAU module - but I want all Google Analytics calls encapsulated within GAU and am not sure how else to propagate the click event into the GAU module.

  3. Similarly, I'm setting the handler for window.onbeforeunload within the GAU module - is this a bad practice? Is it better to set the handler somewhere else and define the handler itself within the GAU module?

  4. I am also interested in reviewers' thoughts on whether I should bother keeping all the Google Analytics event strings encapsulated in the object literals... each one is currently only referenced from one place, all within the GAU module with the exception of the two references within the Razor view. My concern is that the call to the ga function is now a bit messy/unreadable given the way I've structured the FormEvents object.

What do you think?

Any other feedback on how I am using the Module pattern is welcome.

NB: I have put an ellipsis '...' anywhere I have omitted irrelevant code for brevity.

googleAnalyticsUtils.js:

var GAU = (function () {
    var publicInterface = {};        

    var formEvents = {
        Category: 'Form',
        Submit: {
            Action: 'Submit',
            Address: {
                Label: 'Address Submit',
                Value: 1
            },
            Nmi: {
                Label: 'NMI Submit',
                Value: 1
            }
        },
        LeftSubmittedForm: {
            Action: 'LeftSubmittedForm',
            Label: 'Left Form after successful submit',
            Value: 1
        },
        AbandonedForm: {
            Action: 'AbandonedForm',
            Label: 'Left Form without submitting',
            Value: 1
        },
        AnotherReport: {
            Action: 'AnotherReport',
            Label: 'Report Another Outage',
            Value: 1
        }
    };

    var _submitClicked = false;
    var submitClicked = function () {
        _submitClicked = true;
    }

    var registerGoogleAnalyticsTriggers = function () {

        var _reportAnotherOutageClicked = false;
        $("#btn-another-report").click(function () {
            ga('send',
            'event',
            GAU.FormEvents.Category,
            GAU.FormEvents.AnotherReport.Action,
            GAU.FormEvents.AnotherReport.Label,
            GAU.FormEvents.AnotherReport.Value);
            _reportAnotherOutageClicked = true;
        });

        window.onbeforeunload = function () {
            // We dont want to fire these events WHEN the 'Submit' or 'Report Another Outage' buttons have been clicked
            // ...only when the user leaves the page either AFTER 'Submit' has been clicked and the page has reloaded 
            // OR without clicking submit at all
            if (!_submitClicked && !_reportAnotherOutageClicked) {
                if ($(".success").is(":visible")) {
                    ga('send',
                    'event',
                    GAU.FormEvents.Category,
                    GAU.FormEvents.LeftSubmittedForm.Action,
                    GAU.FormEvents.LeftSubmittedForm.Label,
                    GAU.FormEvents.LeftSubmittedForm.Value);
                } else {
                    ga('send',
                    'event',
                    GAU.FormEvents.Category,
                    GAU.FormEvents.AbandonedForm.Action,
                    GAU.FormEvents.AbandonedForm.Label,
                    GAU.FormEvents.AbandonedForm.Value);
                }
            }
        };

    publicInterface.RegisterGoogleAnalyticsTriggers = registerGoogleAnalyticsTriggers;
    publicInterface.SubmitClicked = submitClicked;
    publicInterface.FormEvents = formEvents;

    return publicInterface;
}());

Razor View page that uses the JavaScript:

<html>
    <body>
    ...

@if (Model.Success)
{
    <a id="btn-another-report" class="button">Report another outage</a>
    <form id="ReportAnotherOutageForm" action="/OutageReport/ReportAnOutage" method="post">
        <input id="AnotherOutage" name="AnotherOutage" type="hidden" value="true">
    </form>
}else{
    <form id="ReportOutageForm" class="outage-report_form" action="/OutageReport/ReportAnOutage" method="post" autocomplete="off">
        <button id="btn-submit" type="button" name="btn-submit" class="button">Submit</button>
    </form>
}

    <script type="text/javascript" src="/Scripts/components/googleAnalyticsUtils.js"></script>
    <script type="text/javascript" src="/Scripts/components/reportAnOutage.js"></script>
@if (Model.Outage.IsSubmitted && Model.Outage.Method == ReportingMethod.Address)
{
    <script type="text/javascript">
        // GA event for successful Address form submit
        ga('send',
            'event',
            GAU.FormEvents.Category,
            GAU.FormEvents.Submit.Action,
            GAU.FormEvents.Submit.Address.Label,
            GAU.FormEvents.Submit.Address.Value);
    </script>
}
else if (Model.Outage.IsSubmitted && Model.Outage.Method == ReportingMethod.NMI)
{
    <script type="text/javascript">
        // GA event for successful NMI form submit
        ga('send',
            'event',
            GAU.FormEvents.Category,
            GAU.FormEvents.Submit.Action,
            GAU.FormEvents.Submit.Nmi.Label,
            GAU.FormEvents.Submit.Nmi.Value);
    </script>
}
    </body>
</html>

reportAnOutage.js:

$(function() {

    ...
    $("#btn-submit").click(function (e) {
        ...
        GAU.SubmitClicked();
    });

    // report another outage 
    $("#btn-another-report").click(function () {
        $("#ReportAnotherOutageForm").submit();
    });

    // Google Analytics
    GAU.RegisterGoogleAnalyticsTriggers();
});
\$\endgroup\$
  • \$\begingroup\$ This question is incomplete. To help reviewers give you better answers, please add sufficient context to your question. The more you tell us about what your code does and what the purpose of doing that is, the easier it will be for reviewers to help you. Questions should include a description of what the code does. \$\endgroup\$ – Phrancis Nov 2 '17 at 1:46
  • \$\begingroup\$ @Phrancis I've tried to give as much context as needed...can you please let me know which parts don't make sense to you and I will attempt to make it clearer? \$\endgroup\$ – JTech Nov 2 '17 at 4:57
  • 2
    \$\begingroup\$ The title change in Rev 4 is actually required by the site's How to Ask guidelines. Please don't revert it. \$\endgroup\$ – 200_success Nov 9 '17 at 6:15
1
\$\begingroup\$
  1. The way I'm tracking if the 'Submit' button has been clicked within the GAU module

Well I don't know of any principle that this violates but it might be better to add the click handlers in one spot - perhaps have a public method exposed by the GAU and then bind that to an event listener in reportAnOutage.js - kind of like the first event handler for the click on $("#btn-submit").

  1. I'm handling the click event for the #btn-another-report button twice...once in within reportAnOutage.js and again in the GAU module - but I want all Google Analytics calls encapsulated within GAU and am not sure how else to propagate the click event into the GAU module.

Going along with my response above, it might be cleaner to have the event listeners registered in one place (e.g. reportAnOutage.js)- so that could be bound to a GAU method to a public function that handles the call to ga('send'...).

  1. Similarly, I'm setting the handler for window.onbeforeunload within the GAU module - is this a bad practice? Is it better to set the handler somewhere else and define the handler itself within the GAU module?

Again I don't know of any principles this violates, but I would recommend using EventTarget.addEventListener() for the beforeunload event - that would allow for multiple functions to be executed during that event if such a requirement arose.

  1. Also interested in reviewers' thoughts on whether I should bother keeping all the Google Analytics event strings encapsulated in the object literals... each one is currently only referenced from one place, all within the GAU module with the exception of the two references within the Razor view. My concern is that the call to the ga function is now a bit messy/unreadable given the way I've structured the FormEvents object.

Yes it doesn't hurt. Perhaps it would be beneficial to create a wrapper function to handle the boilerplate, like below. That way, the first three arguments (which appear to be the same throughout the code) to the call to ga() don't have to be repeated every time.

var sendGAEvent = function(action, label, value) {
  ga('send',
        'event',
        GAU.FormEvents.Category,
        action,
        label,
        value)
}

Then just pass the three parameters to that function whenever needed, like below.

\$\endgroup\$

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.