I created a plug-in powered by jQuery that simplifies the developer's life to create responsive applications. It's flexible and I want to give to users a good syntax to write.
How can I improve this?
; (function ($) {
var debug = {
ranges: [],
refreshRanges: function(ranges) {
this.ranges = ranges;
},
invoke: function(reference) {
console.log(this.warnings[reference].apply());
},
clean: function() {
this.ranges = undefined;
},
warnings: {
inRange: function () {
return 'You\'re in range between ' + debug.ranges[0] + 'px and ' + debug.ranges[1] + 'px.';
},
outOfRange: function() {
return 'You\'re out of range between ' + debug.ranges[0] + 'px and ' + debug.ranges[1] + 'px.';
}
}
};
var screen = {
inRange: undefined,
outOfRange: undefined,
firstTimeResponsive: true,
sensibilize: function(ranges, action, callback, debugging) {
if (this.isInRange(ranges) === true) {
this.outOfRange = false;
if (this.inRange === false || (typeof this.inRange === 'undefined' && this.firstTimeResponsive === true)) {
action(device);
this.inRange = true;
this.firstTimeResponsive = false;
if (debugging === true) {
debug.refreshRanges(ranges);
debug.invoke('inRange');
debug.clean();
}
};
} else {
this.inRange = false;
if (this.outOfRange === false || (typeof this.outOfRange === 'undefined' && this.firstTimeResponsive === true)) {
if (this.firstTimeResponsive === false) {
callback(device);
}
this.outOfRange = true;
this.firstTimeResponsive = false;
if (debugging === true) {
debug.refreshRanges(ranges);
debug.invoke('outOfRange');
debug.clean();
}
};
}
},
isInRange: function(ranges) {
if (this.reached(ranges)) {
return true;
};
},
reached: function(range) {
var windowWidth = $(window).width();
if (windowWidth >= range[0] && windowWidth <= range[1]) {
return true;
}
}
};
var device = {
identification: navigator.userAgent,
isMobile: function() {
return /mobi/i.test(navigator.userAgent);
},
Android: function() {
return navigator.userAgent.match(/Android/i);
},
BlackBerry: function() {
return navigator.userAgent.match(/BlackBerry/i);
},
iOS: function() {
return navigator.userAgent.match(/iPhone|iPad|iPod/i);
},
Opera: function() {
return navigator.userAgent.match(/Opera Mini/i);
},
Windows: function() {
return navigator.userAgent.match(/IEMobile/i);
},
any: function() {
return (device.Android() || device.BlackBerry() ||
device.iOS() || device.Opera() || device.Windows());
}
};
$.sensitive = function(ranges, action, callback, options) {
var defaults = {
debugging: false,
handheldDevicesOnly: false,
ultimateScreenWidth: 15360
};
var plugin = this;
plugin.settings = $.extend({}, defaults, options);
if (typeof ranges[1] === 'undefined') {
ranges[1] = plugin.settings.ultimateScreenWidth;
};
if (plugin.settings.handheldDevicesOnly === true && device.isMobile() === false) {
return null;
};
$(window).on('resize', function() {
screen.sensibilize(ranges, action, callback, plugin.settings.debugging);
}).trigger('resize');
};
}(jQuery));
2 Answers 2
My 2 cents :
- Why jQuery, libraries targetting mobile should not require jQuery, #perfmatters
- Your github project should have a production version where debug is excluded, your code is 4215 bytes with debug feature, 3014 without debugging ( removing all debugging related code )
- Your code maintains both the value of
inRange
andoutOfRange
, you should only need 1 ? this.firstTimeResponsive === true
can be replaced withthis.firstTimeResponsive
this.inRange === false || (typeof this.inRange === 'undefined')
can be!this.inRange
isInRange
seems pointless, it's a wrapper aroundreached
without added value ?ranges
is an unfortunate parameter name, since you only pass 1 range ( from/to pair )action
andcallback
are misleading as parameter names, how aboutinRangeCallback
andoutOfRangeCallback
?
From a design perspective, why do you track firstTimeResponse
and also track resizing of the browser, it seems pointless. I think you need to test resizing.
Whereas this review seems harsh, I think the library can be put to good use, but it needs some serious clean up.
-
\$\begingroup\$ Do you think that
debugging
parameter is necessary? I mean, isn't an argument, it's just an option.sensibilize: function(range, inRangeCallback, outOfRangeCallback, debugging) { ... }
\$\endgroup\$Guilherme Oderdenge– Guilherme Oderdenge2013年12月18日 10:29:20 +00:00Commented Dec 18, 2013 at 10:29 -
\$\begingroup\$ It's 25% less code without debugging code. \$\endgroup\$konijn– konijn2013年12月18日 12:00:35 +00:00Commented Dec 18, 2013 at 12:00
-
\$\begingroup\$ I followed your hint — I removed debugging. If you want to see the new version, jump in. \$\endgroup\$Guilherme Oderdenge– Guilherme Oderdenge2013年12月19日 11:52:35 +00:00Commented Dec 19, 2013 at 11:52
I know names aren't hugely important but my 2 cents in addition to having already written some of what @tomdemuyt said (which I editted out):
clean
isn't very descriptive. It's also inconsistent. Based upon other names, a better fit would beclearRanges
You don't always have to do
=== false
or=== true
Please use truthy/falsiness. You're already doing this by not returning a value from your functions that either return true, or undefined. Undefined being falsy.As tomdemuyt said, you can simplify some of your functions. Instead of
if (windowWidth >= range[0] && windowWidth <= range[1]) { return true; }
You could use:
return windowWidth >= range[0] && windowWidth <= range[1];
I agree with tomdemuyt that requiring this to be used with jQuery seems to contradict your description of it being "lightweight". Most of the code doesn't reference jQuery, I'd look into factoring out the jQuery dependence.