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

Best practice javascript objects

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

Problem

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 = "";
        $.each(items, function (i, tweet) {
          var parser = new TwitterTimeline.Parser(tweet.text);
          parser.linkifyURLs();
          parser.linkifyHashTags();
          tweets += "" + tweet.user.name + "" + parser.getHTML() + "";
          if(i === (data.limit - 1)) {
            return false;
          }
        });
        tweets += "";
        $(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, '$1');
    };
    this.linkifyHashTags = function () {
      html = html.replace(hashTagRegex, '#$1');
    };

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

  }
}

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


And my html



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

Solution

//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 = '{{#tweets}}{{user.name}}{{parsedHTML}}{{/tweets}}';

  //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, '$1')
               .replace(hashTagRegex,'#$1');
  }

  //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 = '{{#tweets}}{{user.name}}{{parsedHTML}}{{/tweets}}';

  function parse(html) {
    return html.replace(urlRegex, '$1')
               .replace(hashTagRegex,'#$1')
  }

  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 || {}));

Code Snippets

//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 || {}));
;(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 || {}));

Context

StackExchange Code Review Q#27350, answer score: 2

Revisions (0)

No revisions yet.