patternjavascriptMinor
Display Twitter timelines and lists on a website
Viewed 0 times
websitetimelineslistsandtwitterdisplay
Problem
I've built a Twitter plugin recently and was wondering if I could get some feedback on it. There is also a PHP side that grabs the actual tweets and I can post that code if it's needed.
http://jsfiddle.net/cKfDd/
```
;(function ( $, window, document, undefined ) {
/***
Plugin Functions
***/
/***/
Plugin.prototype.init = function () {
var params = [ '?user=' + this.options.user,
'&limit=' + this.options.limit,
'&type=' + this.options.type,
'&slug=' + this.options.slug,
'&cache=' + this.options.cache,
'&expire=' + this.options.expire,
'&clear=' + this.options.clear,
'&retweets=' + this.options.retweets
].join('\n');
$.ajax({
url: this.options.path_to_core + 'invisibletweets.php' + params,
context: this,
dataType: 'json',
success: function(data){
// If the errors object is returned, show the error and exit the script
if(data.errors){
$(this.element).html("Fatal Error: " + data.errors[0].message);
return false;
}
// Variables
var scope = this;
var parent = this.element;
var tpl = "";
var tplArray = scope.options.template.replace(/[\s,]+/g, ',').split(',');
// Creates an empty template with all present data, in order.
$.each(tplArray, function(i, elm){
if(elm == 'name'){
tpl = tpl + '';
} else if(elm == 'avatar'){
tpl = tpl + '';
} else if(elm == 'date'){
tpl = tpl + '';
} else if(elm == 'time'){
tpl = tpl + '';
} else if(elm == 'text'){
http://jsfiddle.net/cKfDd/
```
;(function ( $, window, document, undefined ) {
/***
Plugin Functions
***/
/***/
Plugin.prototype.init = function () {
var params = [ '?user=' + this.options.user,
'&limit=' + this.options.limit,
'&type=' + this.options.type,
'&slug=' + this.options.slug,
'&cache=' + this.options.cache,
'&expire=' + this.options.expire,
'&clear=' + this.options.clear,
'&retweets=' + this.options.retweets
].join('\n');
$.ajax({
url: this.options.path_to_core + 'invisibletweets.php' + params,
context: this,
dataType: 'json',
success: function(data){
// If the errors object is returned, show the error and exit the script
if(data.errors){
$(this.element).html("Fatal Error: " + data.errors[0].message);
return false;
}
// Variables
var scope = this;
var parent = this.element;
var tpl = "";
var tplArray = scope.options.template.replace(/[\s,]+/g, ',').split(',');
// Creates an empty template with all present data, in order.
$.each(tplArray, function(i, elm){
if(elm == 'name'){
tpl = tpl + '';
} else if(elm == 'avatar'){
tpl = tpl + '';
} else if(elm == 'date'){
tpl = tpl + '';
} else if(elm == 'time'){
tpl = tpl + '';
} else if(elm == 'text'){
Solution
The code over all looks nice and clean and is very readable.
What I'm not really a fan of is the handling of the template.
-
Personally I'd prefer to define the template directly as an array instead of a comma separated string.
-
The big if block building the HTML should be replaced with an object (hash map) with the HTML snippets assigned to each keyword.
-
String concatenations are slow and memory hogs. It would be better to collect the string in an array and
-
It's a bit strange to add the template to the page first and then search for it again before filling it. It would be much easier to first fill the template and then add it to the page.
-
Generally I'd join building and filling the template into one:
Here some (untested) code showing what I mean:
What I'm not really a fan of is the handling of the template.
-
Personally I'd prefer to define the template directly as an array instead of a comma separated string.
-
The big if block building the HTML should be replaced with an object (hash map) with the HTML snippets assigned to each keyword.
-
String concatenations are slow and memory hogs. It would be better to collect the string in an array and
.join('') them.-
It's a bit strange to add the template to the page first and then search for it again before filling it. It would be much easier to first fill the template and then add it to the page.
-
Generally I'd join building and filling the template into one:
Here some (untested) code showing what I mean:
var tplArray = ['name', 'avatar'];
var templateItems = {
name: {
template: '',
fill: function(element, itemData) {
element.find("a").attr("href", itemData.profile_url).html(itemData.user.name);
}
},
avatar: {
template: '',
fill: function(element, itemData) {
element.find('a').attr("href", itemData.profile_url).find('img').attr("src", itemData.user.profile_image_url);
}
}
// etc.
}
$.each(data, function(i, item){
// Put formated data into back into item
item.text = scope.findLinks(item.text);
item.date = scope.dateTime(item.created_at, 'date');
item.time = scope.dateTime(item.created_at, 'time');
item.profile_url = 'http://twitter.com/' + item.user.screen_name;
item.thisTweet = ".it_tweet:eq("+i+") ";
var templateItems = $.map(tplArray, function(i, tplItem) {
var templateData = templateItems[tplItem];
var element = $(templateData.template);
templateData.fill(element, item);
return element;
});
$(parent).append($('').append(templateItems));
});Code Snippets
var tplArray = ['name', 'avatar'];
var templateItems = {
name: {
template: '<div class="it_name"><a href="" target="_blank"></a></div>',
fill: function(element, itemData) {
element.find("a").attr("href", itemData.profile_url).html(itemData.user.name);
}
},
avatar: {
template: '<div class="it_avatar"><a href="" target="_blank"><img src="" /></a></div>',
fill: function(element, itemData) {
element.find('a').attr("href", itemData.profile_url).find('img').attr("src", itemData.user.profile_image_url);
}
}
// etc.
}
$.each(data, function(i, item){
// Put formated data into back into item
item.text = scope.findLinks(item.text);
item.date = scope.dateTime(item.created_at, 'date');
item.time = scope.dateTime(item.created_at, 'time');
item.profile_url = 'http://twitter.com/' + item.user.screen_name;
item.thisTweet = ".it_tweet:eq("+i+") ";
var templateItems = $.map(tplArray, function(i, tplItem) {
var templateData = templateItems[tplItem];
var element = $(templateData.template);
templateData.fill(element, item);
return element;
});
$(parent).append($('<div class="it_tweet"></div>').append(templateItems));
});Context
StackExchange Code Review Q#29967, answer score: 2
Revisions (0)
No revisions yet.