To use or not to use public variables in javascript? Question:



I'm attemping to modify a Javascript snippet to more efficiently get the user's current Zipcode. One problem that I am facing however is the lack of understanding around public functions. I am wondering if they are requried, and if they are not, then what would make the code cleaner and more efficient in terms of using or not using public.? 

Code:



var geoLocateAddress = function () {
        var public = {};  
        public.init = function() {
            public.initEventHandlers();
        };
        public.initEventHandlers = function() {
            console.log("Using your browser's GeoLocation API");
            navigator.geolocation.getCurrentPosition(
            public.loadAddressByGeolocation);
        };
        public.loadAddressByGeolocation = function(position) {
            console.dir(position);
            console.log("Your browser only returns Long/Lat");    
            public.loadAddressByLngLat(position);
        };  
        public.loadAddressByLngLat = function(position){
            var data = {
                lat : position.coords.latitude,
                lng : position.coords.longitude
            };
            var url = "http://ws.geonames.org/findNearestAddressJSON";
            $.ajax({
                url: url,
                dataType: 'jsonp',
                data: data,
                success: function(data, textStatus, xhr) {
                    public.updateAddressFields({
                        zipCode: data.address.postalcode,
                    });
                },
                error: function(xhr, textStatus, errorThrown) {
                    console.log(textStatus);
                }
            });
            return false;
        };
        public.updateAddressFields = function(address) {
            console.log('Zip: ' + address.zipCode);
            $("#zip").text(address.zipCode);
            console.log('Zip Logged');

        };  
        return public;
    } ();
geoLocateAddress.init();
None of this code needs to create any global variables. The pattern you have here is known as namespace creation.

It often follows the pattern:

var myNamespace = (function () {
    //private stuff
    var public = {}; //or function () ...
    //build up public object
    return public;
}());


But sometimes the public object can be inlined like it can be in your case:

var geoLocateAddress = (function () {
    function init() {
        //...
    }
    return { init: init };//unnecessary to expose the other functions as public
}());


In this case, all you are doing with this namespaced global object is calling the init function. So instead of creating a global (generally considered bad taste in most languages) you can simply initialize the object:

(function () {
    function init() {
        //...
    }

    init();//unnecessary to expose the other functions as public
}());


(this pattern of having the function surrounding your code like this is commonly referred to as an IIFE)

Aside: for declaring a namespace I prefer the following form:

(function ($, window) {
    'use strict';

    ...
    window['geoLocateAddress'] = window['geoLocateAddress'] || {};
    window['geoLocateAddress'] = $.extend({}, window['geoLocateAddress'], {
        'init': init
    };
}(jQuery, window));


Back on topic:

In my opinion, it is a bad practice to force the Javascript engine to go looking for your globals when you are not in global scope due to leaving off the window qualifier. Thus everywhere you reference console without calling it window.console (and the same goes for Navigator) or caching it in a local variable is wrong. In the event where you run this script outside of a browser that implements console you will get an exception. Navigator is slightly less of an issue because it is a reasonably safe assumption that you will not run this from a browser that doesn't supply a Navigator global namespace. For this reason I always pass the globals that I am going to use into my IIFE:

(function ($, geolocation, console) {
    ...
}(jQuery, Navigator.geolocation, window.console));


I would modify the rest of the code as follows; please comment if you want me to explain any of it in more detail and I'll try to get to it. Comments dictionary:


pref: personal preference (the opinions of others may vary)
rule: avoids possible issues with the code
possibility: potential source for further enhancement, outside of the scope of this question.
consideration: something I haven't decided one way or the other on yet




//pref: don't create global variables that are unnecessary
(function ($, geolocation, console) {//pref: pass in globals for munging purposes
    'use strict'; //pref: 100% on JsLint

    //pref: instead of defining one object variable and methods on it
    //we could define the methods and export an object at the end
    //this saves us from needing to do some sort of static object call syntax
    var updateAddressFields = function (data) { //pref: remove unused parameters
            console.log('Zip: ' + data.address.postalCode); 
            //pref: renaming variable provides very little benefit; 
            //outweighed by ability to simplify syntax by inlining to success 
            //function and then replacing the success function here
            console.dir(data); //pref: make above a constant string and use this instead of just logging zipcode
            $(function () {  //rule: don't update DOM until after the ready event
                $('#zip').text(data.address.postalCode);
            });
            //pref: further logging beyond start of function adds nothing but cruft to your log
        },
        loadAddressByLocation = function (position) { //pref: renamed function for readability
            console.log('loadAddressByLocation:');
            console.dir(position); //pref: inlined function to simplify code
            var data = { //possibility: abstract this as a jQuery helper if it has uses outside of setting zipcode (outside of this question scope)
                    lat : position.coords.latitude,
                    lng : position.coords.longitude
                },
                url = "http://ws.geonames.org/findNearestAddressJSON"; //consideration: variable unnecessary?
            $.ajax({ //consideration: ajax vs getJSON?
                url: url,
                dataType: 'jsonp', //consideration: specify datatype or put callback in the url?
                data: data, //consideration: inline data definition?
                success: updateAddressFields, //pref: extra function wrapping unnecessary
                error: function (xhr, textStatus) {//perf: remove unused parameters
                    console.log(textStatus);//consideration: necessity of error function?
                }
            });
            //pref: return false did nothing here; removed
        };

    console = console || { //rule: safe to remove shim if you know you're browser compat
        log: function () {}, //otherwise console may not exist under certain browsers
        dir: function () {}
    };
    //perf: removed and inlined "return { init: init };" to not create globals
    console.log("Using your browser's GeoLocation API");//pref: inlined init and initEventHandlers for readability
    if (geolocation &amp;&amp; geolocation.getCurrentPosition) { //rule: more behavior detection, also safe to remove if browser compat
        //assume correct api if this passes
        geolocation.getCurrentPosition(loadAddressByLocation);
    }
}(jQuery, navigator.geolocation, window.console));
//pref: passing in navigator.geolocation will allow better minification (though I wouldn't minify this version anyway)
//rule: more behavior detection, referring to console as window.console to not trigger possible error referencing global that doesn't exist


Simplified for minification, I don't really see anything "wrong" with this version:

(function ($, geolocation) { //console removed
    if (geolocation) { //possibly unsafe if future browser implements geolocation but not getCurrentPosition, perhaps leave this as it was above
        geolocation.getCurrentPosition(function (position) {
            $.getJSON( //using this helper instead of ajax lets us not define an object and property names
                "http://ws.geonames.org/findNearestAddressJSON?callback=?",
                //dataType: 'jsonp', unnecessary with callback in url
                { //inlined data to remove var, IMO readability is not affected too much
                    lat : position.coords.latitude,
                    lng : position.coords.longitude
                },
                function (data) { //unused params dropped from success callback
                    $(function () {
                        //don't update until after the ready event
                        $('#zip').text(data.address.postalcode);
                    });
                }
            );
        });
    }
}(jQuery, navigator.geolocation));


which shrinks down to 266 chars (via uglifyjs):

(function(e,t){t&amp;&amp;t.getCurrentPosition(function(t){e.getJSON(
"http://ws.geonames.org/findNearestAddressJSON?callback=?",{lat:
t.coords.latitude,lng:t.coords.longitude},function(t){e(function(){
e("#zip").text(t.address.postalcode)})})})})(jQuery,navigator.geolocation)