3
\$\begingroup\$

I am basically looking for Critique on the following code as i know its not the best and i am trying to find a set way of building javascript objects.

Be brutal as you way its a learning tool for me, basically i am pulling in a twitter feed via json and adding it to the dom.

var TwitterTimeline = {
  twitterFeed: function (data) {
    $.ajax({
      url: 'http://example.com/api/twitter/timeline/name/' + data.name,
      dataType: 'json',
      crossDomain: true,
      cache: false,
      success: function (items) {
        var tweets = "<ul>";
        $.each(items, function (i, tweet) {
          var parser = new TwitterTimeline.Parser(tweet.text);
          parser.linkifyURLs();
          parser.linkifyHashTags();
          tweets += "<li><h6><a href=''>" + tweet.user.name + "</a></h6>" + parser.getHTML() + "</li>";
          if(i === (data.limit - 1)) {
            return false;
          }
        });
        tweets += "</ul>";
        $(data.id).html(tweets);
      }
    });
  },
  Parser: function (text) {

    var html = text;

    var urlRegex = /((ftp|http|https):\/\/(\w+:{0,1}\w*@)?(\S+)(:[0-9]+)?(\/|\/([\w#!:.?+=&%@!\-\/]))?)/gi;
    var hashTagRegex = /#([^ ]+)/gi;

    this.linkifyURLs = function () {
      html = html.replace(urlRegex, '<a href="$1">$1</a>');
    };
    this.linkifyHashTags = function () {
      html = html.replace(hashTagRegex, '<a href="http://twitter.com/#!/search?q=%23$1">#$1</a>');
    };

    this.getHTML = function () {
      return html;
    };

  }
}

TwitterTimeline.twitterFeed({
  id: "#twitter-feed",
  name: 'testaccount',
  limit: 2
});

And my html

<div id="twitter-feed" class="twitter-feed"></div>

Any issues or bad practise which i bet there is a lot please let me know. So many different way to do things with javascript i always feel like i am hacking things together without any structure.

Thanks

\$\endgroup\$

1 Answer 1

2
\$\begingroup\$
 //Let's wrap this module in a closure so we can have a private scope for our
 //module. That way, we don't expose too many globals as well as place too
 //many things in our namespace
;
(function (exports) {

  //Let's pull out these guys since they are static, and we don't want them
  //get recreated everytime a parse is required.
  var urlRegex = /((ftp|http|https):\/\/(\w+:{0,1}\w*@)?(\S+)(:[0-9]+)?(\/|\/([\w#!:.?+=&%@!\-\/]))?)/gi;
  var hashTagRegex = /#([^ ]+)/gi;

  //I suggest you use a templating system. In this example, I am using
  //Mustache. As you can see, it's easily readable, where you have an
  //array of tweets, each printing the contained section.
  var feedTemplate = '<ul>{{#tweets}}<li><h6><a href="">{{user.name}}</a></h6>{{parsedHTML}}{{/tweets}}</ul>';

  //Now our parser doesn't need to be an object. It could only be a function
  //that returns parsed data. Also, unless parse is required to be public, it
  //should remain inside the scope that requires it. That way, we don't
  //expose too much to our namespace.

  function parse(html) {
    return html.replace(urlRegex, '<a href="$1">$1</a>')
               .replace(hashTagRegex,'<a href="http://twitter.com/#!/search?q=%23$1">#$1</a>');
  }

  //We move out this function to separate it from the AJAX call. That way,
  //it's not jumbled up with the AJAX code.

  function format(data, config) {

    //We have an array of twets that we will use later with the templating.
    var tweets = [];

    $.each(data, function (i, tweet) {
      //Now, for each tweet retrieved, we parse the text, and reattach it to
      //the tweet object as parsedHTML, which will be used in the template.
      tweet.parsedHTML = parse(tweet.text);

      //We then collect the tweets into the array defined earlier.
      tweets.push(tweet);

      //We needed that array, so that we won't feed the original array into
      //the template. So here's the break when the limit is reached.
      if(i === config.limit) return false;
    });

    //Now, with the template, and our tweet array, we render. The return is
    //an HTML string, which we can then use jQuery to build the elements.
    var html = Mustache.render(feedTemplate, {
      tweets: tweets
    });

    $(data.id).html(html);
  }

  //We expose our twitterFeed function to our namespace
  exports.twitterFeed = function (config) {
    $.ajax({
      url: 'http://example.com/api/twitter/timeline/name/' + data.name,
      dataType: 'json',
      crossDomain: true,
      cache: false,
      success: function (data) {
        format(data, config);
      }
    });
  }

}(this.TwitterTimeline = this.TwitterTimeline || {}));

Minus the comments, this is how it looks:

;(function (exports) {
  var urlRegex = /((ftp|http|https):\/\/(\w+:{0,1}\w*@)?(\S+)(:[0-9]+)?(\/|\/([\w#!:.?+=&%@!\-\/]))?)/gi;
  var hashTagRegex = /#([^ ]+)/gi;
  var feedTemplate = '<ul>{{#tweets}}<li><h6><a href="">{{user.name}}</a></h6>{{parsedHTML}}{{/tweets}}</ul>';

  function parse(html) {
    return html.replace(urlRegex, '<a href="$1">$1</a>')
               .replace(hashTagRegex,'<a href="http://twitter.com/#!/search?q=%23$1">#$1</a>')
  }

  function format(data, config) {
    var tweets = [];
    $.each(data, function (i, tweet) {
      tweet.parsedHTML = parse(tweet.text);
      tweets.push(tweet);
      if(i === config.limit) return false
    });
    var html = Mustache.render(feedTemplate, {
      tweets: tweets
    });
    $(data.id).html(html)
  }

  exports.twitterFeed = function (config) {
    $.ajax({
      url: 'http://example.com/api/twitter/timeline/name/' + data.name,
      dataType: 'json',
      crossDomain: true,
      cache: false,
      success: function (data) {
        format(data, config)
      }
    })
  }

}(this.TwitterTimeline = this.TwitterTimeline || {}));
\$\endgroup\$
5
  • \$\begingroup\$ I would probably have the model pull the view from the HTML (for example from a <template> tag, or a <script type='template/Mustache>) rather than store it as a string template. Other than that, I think this solution is very nice and solves most of the problems OP's code has. That $.each` could have been a $.map. \$\endgroup\$ Commented Jun 13, 2013 at 14:56
  • \$\begingroup\$ @BenjaminGruenbaum Nice point. But I used a string instead to represent the fact that it is a string. One can easily use .html() to pull it out from the HTML, or use AJAX if the template is from another file later on. Also, I think $.each() would be better since return false will break the loop. In $.map(), you can return null to remove the item, but the loop will still run through the entire collection. \$\endgroup\$
    – Joseph
    Commented Jun 13, 2013 at 15:12
  • 2
    \$\begingroup\$ I've always viewed the fact you can break an each by returning false as a bug in the implementation. It was considered, and rejected in the spec when building native .map. If you're filtering and then mapping, consider using .filter and then .map. I think it represents the actual action done much better. Also, feedTemplate is not just a string, it's an HTML string. Storing HTML strings in code means not separating your view from your view model, which can get really sucky as you scale. \$\endgroup\$ Commented Jun 13, 2013 at 15:16
  • \$\begingroup\$ @BenjaminGruenbaum Point taken on the HTML string and code semantics. However, on the performance department, filter+map is slower due to the fact that it equates to running 2 nonbreaking loops, generating at least 2 more objects as seen in this test (Firefox24 and Chromium25 seem to agree). \$\endgroup\$
    – Joseph
    Commented Jun 13, 2013 at 15:49
  • 1
    \$\begingroup\$ Are we really having a discussion on which is faster? Performance would be meaningless in this case (Or do you intend to run over 30K arrays with 100 elements through it every second?). If you're worried about performance, all three options are very slow compared to a native loop by at least a factor of 10. \$\endgroup\$ Commented Jun 13, 2013 at 15:54

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.