Optimise a function in Javascript

Question

I'm quite new to javascript, but have managed to write a working xml function :)

I was hoping someone could please give me a rundown of how to optimise the function. At present there's a different function for each state's weather, but I was hoping I could simplify this somehow.

Code is pasted here: http://pastie.org/private/ffuvwgbeenhyo07vqkkcsw

Any help is greatly appreciated. Thank you!

EDIT: ADDING CODE EXAMPLES OF BOTH XML FEEDS:

Function 1 (UV): http://pastie.org/private/jc9oxkexypn0cw5yaskiq

Function 2 (weather): http://pastie.org/private/pnckz4k4yabgvtdbsjvvrq

Answer 1

$(d).find('location#sydney')

Is questionable. #sydney means an element with an attribute valued sydney that has the schema type ID. In HTML, the id="..." attribute has schema type ID thanks to the DOCTYPE. But this XML file has no DOCTYPE and hence its id="..." attributes don't have schema type ID. Consequently getElementById('sydney') won't work, and #sydney as a selector shouldn't work.

It works in practice because when you use find() jQuery falls back to its own ‘Sizzle’ JavaScript selector matcher, which simply looks for id="..." attributes as if it were HTML. But Sizzle is slow and you shouldn't rely on this implementation detail. Using the explicit attribute selector location[id=sydney] is better for an XML document.

var sydneyuv = sydneyuv += '<span>' + uvindex + '</span>' ;

You've got a superfluous assignment here. You use the augmented assignment += to add something to sydneyuv, and then you assign the result to sydneyuv again.

Also, it's generally best not to stitch together strings of HTML from input values. What if uvindex had HTML-special characters in? (It probably won't, but there's nothing stopping the site you're scraping from including them.) Without HTML-escaping you'd have HTML-injection and potentially XSS security holes. Always use DOM-style methods, like text() and attr() in jQuery, or the creation shortcut: var $sydneyuv= $('<span/>', {text: uvindex});, in preference to string-slinging.

I was hoping I could simplify this somehow.

Sure. Make it data-driven:

var towns= ['sydney', 'melbourne', 'brisbane', 'perth', 'adelaide', 'darwin'];
var uvlevels= [
    {uvlevel: 2,    risk: 'Low',         curcon: 'You can safely stay outdoors and use an SPF 15 moisturiser.'},
    {uvlevel: 5,    risk: 'Moderate',    curcon: 'Wear protective clothing outdoors and use an SPF 15 or SPF 30 moisturiser.'},
    {uvlevel: 7,    risk: 'High',        curcon: 'Wear protective clothing, limit your time outdoors and use an SPF 30 moisturiser.'},
    {uvlevel: 10,   risk: 'Very high',   curcon: 'Use caution, limit exposure to the sun and use an SPF 30 moisturiser.'},
    {uvlevel: 20,   risk: 'Extreme',     curcon: 'Use extreme caution, avoid exposure to the sun and use an SPF 30 moisturiser.'},
    {uvlevel: null, risk: 'Unavailable', curcon: 'Information is currently unavailable.'}
];

Now you can replace all those separate statements with one loop:

$.each(towns, function() {
    var $location= $(d).find('location[id='+this+']');
    var uv= $location.find('index').text();
    var shorttown= this.slice(0, 3);
    $('#uv-'+shortttown).empty().append($('<span/>', {text: uv}));
    $.each(uvlevels, function() {
        if (this.uvlevel===null || uv<=this.uvlevel) {
            $('#risk-'+shorttown).text(this.risk);
            $('#curcon-'+shorttown).text(this.curcon);
            return false;
        }
    });
});

and presumably similar for whatever the weather one's doing.

(I'd use the full town ID in the HTML document IDs, so you don't need the shorttown hack.)

Answer 2

A simplified version would look something like this:

var cities = ['sydney', 'melbourne', 'brisbane', 'perth', 'adelaide', 'darwin'], 
    risks = {
        2: {
            risk: 'Low', 
            curcon: 'You can safely stay outdoors and use an SPF 15 moisturiser.'
        }
        5: {
            risk: 'Moderate', 
            curcon: 'Wear protective clothing outdoors and use an SPF 15 or SPF 30 moisturiser.'
        }
        7: {
            risk: 'High', 
            curcon: 'Use caution, limit exposure to the sun and use an SPF 30 moisturiser.'
        }
        10: {
            risk: 'Very High', 
            curcon: 'Use caution, limit exposure to the sun and use an SPF 30 moisturiser.'
        }
        20: {
            risk: 'Extreme', 
            curcon: 'Use extreme caution, avoid exposure to the sun and use an SPF 30 moisturiser.'
        }
    };

for(var i = 0; i < cities.length; i++){
    var shortCityName = cities[i].substring(0, 3);

    $(d).find('location#sydney').each(function(){
        var $location = $(this); 
        var uvindex = parseInt($location.find('index').text(), 10);

        $('#uv-' + shortCityName).html('<span>' + uvindex + '</span>');

        for(var i in risks){
            if(uvindex < risks[i]){
                $('#risk-' + shortCityName).html(risks[i].risk);
                $('#curcon-' + shortCityName).html(risks[i].curcon);
            }
        }
    });
}

Using an object to store the messages, then an array to store the names of the cities. Also, instead of using this:

var wicon = wicon += '<img src="' + icon + '" alt="Weather Unavailable" />' ;

You can use this instead:

var wicon = $('<img /').attr({
    src: icon, 
    alt: 'Weather Unavailable'
});

And finally, requesting XML information cross-domain will cause problems. See if the API provides the information in JSONP format instead. JSON is also (slightly) easier to work with using JavaScript.

Answer 3

I would suggest you create an array that holds the UV element IDs, suffixes and weather station ids (stationid)

var areas = [
     {id:'sydney',suffix:'syd',stationid:'YSSY'},
     {id:'melbourne',suffix:'mel',stationid:'YMML'},
     {id:'brisbane',suffix:'bri',stationid:'YBBN'},
     {id:'perth',suffix:'per',stationid:'YPPH'},
     ...
 ]

and then for the UV

// FUNCTION 1 - UV
function getUV() {

    // BEGIN AUSTRALIAN UV FUNCTION

    $.get('http://www.arpansa.gov.au/uvindex/realtime/xml/uvvalues.xml', function(d) {

        //SYDNEY UV
             $(areas).each(function(){
            var area = this;
        $(d).find('location#'+area.name).each(function(){

            var $location = $(this); 
            var uvindex = $location.find('index').text();

            var areauv = areauv += '<span>' + uvindex + '</span>' ;
            $('#uv-'+area.suffix).empty().append($(areauv)); // empty div first

            if (uvindex <= 2.0) {
                $('#risk-'+area.suffix).empty().append('Low');
                $('#curcon-'+area.suffix).empty().append('You can safely stay outdoors and use an SPF 15 moisturiser.');
            } else if (uvindex <= 5.0) {
                $('#risk-'+area.suffix).empty().append('Moderate');
                $('#curcon-'+area.suffix).empty().append('Wear protective clothing outdoors and use an SPF 15 or SPF 30 moisturiser.');
            } else if (uvindex <= 7.0) {
                $('#risk-'+area.suffix).empty().append('High');
                $('#curcon-'+area.suffix).empty().append('Wear protective clothing, limit your time outdoors and use an SPF 30 moisturiser.');
            } else if (uvindex <= 10.0) {
                $('#risk-'+area.suffix).empty().append('Very High');
                $('#curcon-'+area.suffix).empty().append('Use caution, limit exposure to the sun and use an SPF 30 moisturiser.');
            } else if (uvindex <= 20.0) {
                $('#risk-'+area.suffix).empty().append('Extreme');
                $('#curcon-'+area.suffix).empty().append('Use extreme caution, avoid exposure to the sun and use an SPF 30 moisturiser.');
            } else {
                $('#risk-'+area.suffix).empty().append('Unavailable');
                $('#curcon-'+area.suffix).empty().append('Information is currently unavailable.');
            }

        });
            });

        // END OF AUSTRALIAN UV FUNCTION

    });
}

and for the weather

function getWeather() {

        // BEGIN AUSTRALIA and NEW ZEALAND WEATHER FUNCTION
            $(areas).each(function(){
                var area = this;
        $.get('http://api.wxbug.net/getLiveCompactWeatherRSS.aspx?ACode=XXXPRIVATEXXX&stationid='+area.stationid+'&unittype=1&outputtype=1', function(d){
        $(d).find('weather').each(function(){

            var $weatherinfo = $(this); 
            var degrees = $weatherinfo.find('temp').text().replace(/\.0$/i, "");
            var conditions = $weatherinfo.find('current-condition').text();
            var icon = $weatherinfo.find('current-condition').attr('icon').replace(/\.gif$/i, ".png").split('http://deskwx.weatherbug.com/')[1];

            var temperature = temperature += '<span>' + degrees + '</span>' ;   
            $('#temp-'+area.suffix).empty().append($(temperature));

            var winformation = winformation += '<span>' + conditions + '</span>' ;
            $('#info-'+area.suffix).empty().append($(winformation));

            var wicon = wicon += '<img src="' + icon + '" alt="Weather Unavailable" />' ;
            $('#icon-'+area.suffix).empty().append($(wicon));

        });
        });
            });
}

Answer 4

This should give you enough to remove a whole bunch of duplication.

var lte2 = { risk: "Low", curcon: "You can safely stay outdoors and use an SPF 15 moisturiser." },
    lte5 = { risk: "Moderate", curcon: "Wear protective clothing outdoors and use an SPF 15 or SPF 30 moisturiser." },
    lte7 = { risk: "High", curcon: "Wear protective clothing, limit your time outdoors and use an SPF 30 moisturiser." },
    uvWarningMap = {
        0: lte2,
        1: lte2,
        2: lte2,
        3: lte5,
        4: lte5,
        5: lte5,
        6: lte7,
        7: lte7
    };

Answer 5

You need to generalize the function so that each method can support any city. Right now I don't see anything different between what you're doing for Brisbane and Melbourne, for example. The warnings are the same.