15
\$\begingroup\$

What it does is:

  1. Reads from a RSS feed using Google Feed API
  2. Shows the list in an unordered list

How good/bad is the code snippet?

$(document).ready(function(){
var FeedManager = {
        config : {
            feedContainer : $('#feedContainer'),
            feedUrl : 'http://rss.bdnews24.com/rss/english/home/rss.xml',
            feedLimit : 10
        },  
        init: function(){
            var feed = new google.feeds.Feed(FeedManager.config.feedUrl);
            feed.setNumEntries(FeedManager.config.feedLimit) ;
            feed.load(function(result) {
                if (!result.error) {
                    FeedManager.$feedContainer = FeedManager.config.feedContainer;
                    for (var i = 0; i < result.feed.entries.length; i++) {
                        var entry = result.feed.entries[i];
                        $('<li/>').append(
                            $('<a>'+entry.title+'</a>').attr(
                                {
                                    'title': entry.title,
                                    'href': entry.link
                                }
                            ).bind('click',FeedManager.showStory)
                        ).appendTo(FeedManager.$feedContainer);
                    }
                }
                else{
                        FeedManager.handleError(result.error.message);
                }
            });
        },
        showStory: function(){
              var href = event.currentTarget.href;
              FeedManager.showURL(href);
              event.preventDefault();
        },
        showURL: function(url){
             if (url.indexOf("http:") != 0 && url.indexOf("https:") != 0) {
                return;
            }
            chrome.tabs.create({
                url: url
            });
        },
        handleError: function(errorText){
            $('<li/>')
                .css("color","red")
                .append("Error:"+errorText)
                .appendTo(FeedManager.config.feedContainer);
        }
    };

    FeedManager.init();
});

At the 2nd stage, I wanted to add custom accordion feature and news snippet:

processFeedResult: function(result) {
    if (result.error) {
        FeedManager.handleError(result.error.message);
        return;
    }
    FeedManager.$feedContainer = FeedManager.config.feedContainer;
    $.each(result.feed.entries, function() {
        $('<li/>').append(
            $('<a>', {  // this should not be an anchor tag,right? TBD 
                title: 'Published at: '+this.publishedDate,
                text: this.title
            })
        ).append($('<div>', {
            text: this.contentSnippet,
            css : {'display':'none',
                'padding-top':'2px'}
            }).append(
                $('<a>', {
                    text: '[more..]',
                    href: this.link,
                    click: FeedManager.showStory
                })) 
        )
        .bind('click',FeedManager.showSnippet)
        .appendTo(FeedManager.$feedContainer);
    });
},
showSnippet: function() {
    var $obj = $(event.currentTarget),
    $snippetDiv = $obj.find('div').slideDown('normal');

    if(FeedManager.$lastOpenedDiv === undefined){
        FeedManager.$lastOpenedDiv = $snippetDiv ;
    }
    else{
        FeedManager.$lastOpenedDiv.slideUp('normal');
        FeedManager.$lastOpenedDiv = $snippetDiv ;
    }
    }   
};

I feel that I have put some tightly coupled code in my processFeedResult such as text and CSS. I also wanted to know if my showSnippet function good enough or not. However, it works, and I know that there are 3rd party good accordion available, but I wanted to learn it.

So far I've kept the anchor tag to show the news title and I used anchor title as a tooltip that shows the time. Maybe there is a good alternate, like span or paragraph?

\$\endgroup\$
  • \$\begingroup\$ I think that you should take a look at jQuery TMPL. \$\endgroup\$ – John Gietzen Mar 5 '11 at 16:42
  • \$\begingroup\$ you should run it through jsLint. Also, you should use one var statement per function. \$\endgroup\$ – zzzzBov Mar 10 '11 at 18:06
13
\$\begingroup\$

Looks ok, to be honest. A few minor changes I would make:

I would pull this code out into a function

function ProcessFeedResult(result) {
    if (!result.error) {
        FeedManager.$feedContainer = FeedManager.config.feedContainer;
        for (var i = 0; i < result.feed.entries.length; i++) {
            var entry = result.feed.entries[i];
            $('<li/>').append(
                $('<a>'+entry.title+'</a>').attr(
                {
                    'title': entry.title,
                    'href': entry.link
                }
                ).bind('click',FeedManager.showStory)
            ).appendTo(FeedManager.$feedContainer);
        }
    }
    else{
        FeedManager.handleError(result.error.message);
    }
}

Then I would reverse the condition and return, to reduce the level of indent a bit

function ProcessFeedResult(result) {
    if (result.error) {
        FeedManager.handleError(result.error.message);
        return;
    }

    FeedManager.$feedContainer = FeedManager.config.feedContainer;
    for (var i = 0; i < result.feed.entries.length; i++) {
        var entry = result.feed.entries[i];
        $('<li/>').append(
            $('<a>'+entry.title+'</a>').attr(
            {
                'title': entry.title,
                'href': entry.link
            }
            ).bind('click',FeedManager.showStory)
        ).appendTo(FeedManager.$feedContainer);
    }
}

Further, I would replace the loop with jQuery's $.each()

function ProcessFeedResult(result) {
    if (result.error) {
        FeedManager.handleError(result.error.message);
        return;
    }

    FeedManager.$feedContainer = FeedManager.config.feedContainer;
    $.each(result.feed.entries, function() {
        $('<li/>').append(
            $('<a>' + this.title + '</a>').attr(
            {
                'title': this.title,
                'href': this.link
            }
            ).bind('click',FeedManager.showStory)
        ).appendTo(FeedManager.$feedContainer);
    });
}

You can do any or all of these, I don't think any of it is vastly important. I can read your intent easily enough, it just looks a bit neater to me after refactoring.

Thinking about your later comment a bit, I would also be tempted to append the title, rather than stringing it together.

function ProcessFeedResult(result) {
    if (result.error) {
        FeedManager.handleError(result.error.message);
        return;
    }

    FeedManager.$feedContainer = FeedManager.config.feedContainer;
    $.each(result.feed.entries, function() {
        $('<li/>').append(
            $('<a/>').append(this.title).attr(
            {
                'title': this.title,
                'href': this.link
            }
            ).bind('click',FeedManager.showStory)
        ).appendTo(FeedManager.$feedContainer);
    });
}

Otherwise this is the correct approach to generating elements, as far as I know.

As a final step, I would tidy up the braces around the attributes, for consistency

function ProcessFeedResult(result) {
    if (result.error) {
        FeedManager.handleError(result.error.message);
        return;
    }

    FeedManager.$feedContainer = FeedManager.config.feedContainer;
    $.each(result.feed.entries, function() {
        $('<li/>').append(
            $('<a/>').append(this.title).attr({
                'title': this.title,
                'href': this.link
            }).bind('click',FeedManager.showStory)
        ).appendTo(FeedManager.$feedContainer);
    });
}

I really don't think you're going to get it any cleaner than that.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the nice help! No worries,The code worked fine :D , Still I am wondering, how to build the li and anchor tags in a nice way. \$\endgroup\$ – Shahriar Mar 3 '11 at 18:38
  • \$\begingroup\$ @user1234 - See latest edit \$\endgroup\$ – pdr Mar 3 '11 at 19:04
  • \$\begingroup\$ @nezz - Happy to help, it was a good question. Don't forget to vote and accept on the left \$\endgroup\$ – pdr Mar 3 '11 at 20:44
  • \$\begingroup\$ sure,but I still don't have 15 reputation to qualify for voting :( \$\endgroup\$ – Shahriar Mar 3 '11 at 20:51
  • \$\begingroup\$ @pdr - I've added some new features,would you give a look please. \$\endgroup\$ – Shahriar Mar 5 '11 at 21:29
5
\$\begingroup\$

You can use jQuery 1.4's new element creation syntax to shorten things a little (rewriting pdr's answer):

$('<li>').append(
    $('<a>', {
        title: this.title,
        text: this.title,
        href: this.link,
        click: FeedManager.showStory
    })
).appendTo(FeedManager.$feedContainer);

And the same can be done with the element created in the handleError function

\$\endgroup\$
1
\$\begingroup\$

There is no comments.

This:

What it does is:

Reads from a RSS feed using google feed API shows the list in an unordered list.

Should be a comment somewhere in top of the code

\$\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.