HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Improve AJAX response handling function

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
handlingajaxresponsefunctionimprove

Problem

This code handles a response from an RSS feed. It organizes and appends to content. If there's a video embeded then separate it from the rest of the content. I would like a review mainly on performance/efficiency but I'm open to any other suggestions as well. That star selector is really nagging me but I don't know of a better way to iterate over all the contained elements.

function getFeed(url, element, callback) {
    $.getJSON("https://ajax.googleapis.com/ajax/services/feed/load?v=1.0&callback=?&q="+encodeURIComponent(url), function(response) {
        var content = "",
            $element = $(element);

        for (var i = 0; i ");
                $this.attr("width", 640).attr("height", 360).parent().appendTo(element + " .video");
            };
        });

        if (typeof callback === 'function') {
            callback();
        };
    });
}


This is then called like so:

getFeed("http://www.kent.k12.wa.us/site/RSS.aspx?PageID=3854", "#TechExpo", optionalCallback);


Here's what a response might look like

Some text blah blah blah.
Some more text

Video Title

Watch the 7th annual Tech Expo highlights.

Solution

Late reply,

this code is good, I can only find 2 faults:

-
Your code should have error handling for the JSON call, things can go wrong

-
Too much horizontal stretching, I would introduce some sugar for this:

for (var i = 0; i < response.responseData.feed.entries.length; i++) {
    content = content + response.responseData.feed.entries[i].content; //Join all the feed entries
}


could be

var entries = response.responseData.feed.entries;
for (var i = 0; i < entries.length; i++) {
    content = content + entries[i].content; //Join all the feed entries
}


Also, I find

$element.find(".content").html(content)
                         .addClass($element.find("embed").length? "withVideo" : "");


more readable.

As for find("*"), it seems to be cleanest way to access all children.

Code Snippets

for (var i = 0; i < response.responseData.feed.entries.length; i++) {
    content = content + response.responseData.feed.entries[i].content; //Join all the feed entries
}
var entries = response.responseData.feed.entries;
for (var i = 0; i < entries.length; i++) {
    content = content + entries[i].content; //Join all the feed entries
}
$element.find(".content").html(content)
                         .addClass($element.find("embed").length? "withVideo" : "");

Context

StackExchange Code Review Q#28896, answer score: 2

Revisions (0)

No revisions yet.