principlejavascriptMinor
Best practice javascript objects
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.
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
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.