I'm not sure if this is the right way to do this, but I want to simply this script and hopefully use CSS3 animations. The code is really large, and parts are very irrelevant, so I'm wondering if I should first get rid of all the irrelevant parts, then go from there?
Here's the code:
var _target = null,
_dragx = null,
_dragy = null,
_rotate = null,
_resort = null;
var _dragging = false,
_sizing = false,
_animate = false;
var _rotating = 0,
_width = 0,
_height = 0,
_left = 0,
_top = 0,
_xspeed = 0,
_yspeed = 0;
var _zindex = 1000;
jQuery.fn.touch = function (settings) {
// DEFINE DEFAULT TOUCH SETTINGS
settings = jQuery.extend({
animate: false,
sticky: false,
dragx: true,
dragy: true,
rotate: false,
resort: false,
scale: false
}, settings);
// BUILD SETTINGS OBJECT
var opts = [];
opts = $.extend({}, $.fn.touch.defaults, settings);
// ADD METHODS TO OBJECT
this.each(function () {
this.opts = opts;
this.ontouchstart = touchstart;
this.ontouchend = touchend;
this.ontouchmove = touchmove;
this.ongesturestart = gesturestart;
this.ongesturechange = gesturechange;
this.ongestureend = gestureend;
});
};
function touchstart(e) {
_target = this.id;
_dragx = this.opts.dragx;
_dragy = this.opts.dragy;
_resort = this.opts.resort;
_animate = this.opts.animate;
_xspeed = 0;
_yspeed = 0;
$(e.changedTouches).each(function () {
var curLeft = ($('#' + _target).css("left") == 'auto') ? this.pageX : parseInt($('#' + _target).css("left"));
var curTop = ($('#' + _target).css("top") == 'auto') ? this.pageY : parseInt($('#' + _target).css("top"));
if (!_dragging && !_sizing) {
_left = (e.pageX - curLeft);
_top = (e.pageY - curTop);
_dragging = [_left, _top];
if (_resort) {
_zindex = ($('#' + _target).css("z-index") == _zindex) ? _zindex : _zindex + 1;
$('#' + _target).css({
zIndex: _zindex
});
}
}
});
};
function touchmove(e) {
if (_dragging && !_sizing && _animate) {
var _lastleft = (isNaN(parseInt($('#' + _target).css("left")))) ? 0 : parseInt($('#' + _target).css("left"));
var _lasttop = (isNaN(parseInt($('#' + _target).css("top")))) ? 0 : parseInt($('#' + _target).css("top"));
}
$(e.changedTouches).each(function () {
e.preventDefault();
_left = (this.pageX - (parseInt($('#' + _target).css("width")) / 2));
_top = (this.pageY - (parseInt($('#' + _target).css("height")) / 2));
if (_dragging && !_sizing) {
if (_animate) {
_xspeed = Math.round((_xspeed + Math.round(_left - _lastleft)) / 1.5);
_yspeed = Math.round((_yspeed + Math.round(_top - _lasttop)) / 1.5);
}
if (_dragx || _dragy) $('#' + _target).css({
position: "absolute"
});
if (_dragx) $('#' + _target).css({
left: _left + "px"
});
if (_dragy) $('#' + _target).css({
top: _top + "px"
});
$('#' + _target).css({
backgroundColor: "#4B880B"
});
$('#' + _target + ' b').text('WEEEEEEEE!!!!');
}
});
};
function touchend(e) {
$(e.changedTouches).each(function () {
if (!e.targetTouches.length) {
_dragging = false;
if (_animate) {
_left = ($('#' + _target).css("left") == 'auto') ? this.pageX : parseInt($('#' + _target).css("left"));
_top = ($('#' + _target).css("top") == 'auto') ? this.pageY : parseInt($('#' + _target).css("top"));
var animx = (_dragx) ? (_left + _xspeed) + "px" : _left + "px";
var animy = (_dragy) ? (_top + _yspeed) + "px" : _top + "px";
if (_dragx || _dragy) $('#' + _target).animate({
left: animx,
top: animy
}, "fast");
}
}
});
$('#' + _target + ' b').text('I am sad :(');
$('#' + _target).css({
backgroundColor: "#0B4188"
});
setTimeout(changeBack, 5000, _target);
};
function gesturestart(e) {
_sizing = [$('#' + this.id).css("width"), $('#' + this.id).css("height")];
};
function gesturechange(e) {
if (_sizing) {
_width = (this.opts.scale) ? Math.min(parseInt(_sizing[0]) * e.scale, 300) : _sizing[0];
_height = (this.opts.scale) ? Math.min(parseInt(_sizing[1]) * e.scale, 300) : _sizing[1];
_rotate = (this.opts.rotate) ? "rotate(" + ((_rotating + e.rotation) % 360) + "deg)" : "0deg";
$('#' + this.id).css({
width: _width + "px",
height: _height + "px",
webkitTransform: _rotate
});
$('#' + this.id + ' b').text('TRANSFORM!');
$('#' + this.id).css({
backgroundColor: "#4B880B"
});
}
};
function gestureend(e) {
_sizing = false;
_rotating = (_rotating + e.rotation) % 360;
};
function changeBack(target) {
$('#' + target + ' b').text('Touch Me :)');
$('#' + target).css({
backgroundColor: "#999"
});
}
2 Answers 2
I'm wondering if I should first get rid of all the irrelevant parts
Yes. Especially since source control will keep any code you may want in the future. If it is not being used it is just taking up resources to maintain it. (e.g. everytime you read this section of code you have to remember what is and isn't relevant.)
If these are Constants I would suggest capitalising them. then your code will read a little easier. If they are default settings put them in an object. (If they are global variables...)
var _target = null,
_dragx = null,
_dragy = null,
_rotate = null,
_resort = null;
var _dragging = false,
_sizing = false,
_animate = false;
var _rotating = 0,
_width = 0,
_height = 0,
_left = 0,
_top = 0,
_xspeed = 0,
_yspeed = 0;
var _zindex = 1000;
your style uses _variablename but javascript normally has the camelCase style for variables, CamelCase for classes and previously mentioned ALLCAPS for constants. Consistency is good but unless there is a reason not to i would suggest sticking with standards. (esp if others maintain it after you)
_left = (e.pageX - curLeft);
_top = (e.pageY - curTop);
_dragging = [_left, _top];
statements like:
$('#' + _target).css({
backgroundColor: "#4B880B"
});
$('#' + _target + ' b').text('WEEEEEEEE!!!!');
can be chained like:
$('#' + _target)
.css({
backgroundColor: "#4B880B"
})
.find('b')
.text('WEEEEEEEE!!!!');
When you use the same selector more than once its best to put it in a variable so you don't make jQuery work harder.
if (_dragx || _dragy) $('#' + _target).css({
position: "absolute"
});
if (_dragx) $('#' + _target).css({
left: _left + "px"
});
if (_dragy) $('#' + _target).css({
top: _top + "px"
});
can become:
var $target = $('#' + _target);
if (_dragx || _dragy) $target.css({
position: "absolute"
});
if (_dragx) $target.css({
left: _left + "px"
});
if (_dragy) $target.css({
top: _top + "px"
});
the if form you use here is unusual.
if (_dragy) $target.css({
top: _top + "px"
});
Might just be a matter of taste but I find it puts greater cognitive load (make me think too hard) when there are two statements per line:
if(_dragy)
{
$target.css({
top: _top + "px"
});
}
Achieves the same and is much easier to read through.
Building upon James' Answer, you should also collapse the multiple css calls into one call:
// touchmove
cssObject = { backgroundColor: "#4b880b" };
if (_dragx || _dragy) cssObject.position = "absolute";
if (_dragx) cssObject.left = _left + "px";
if (_dragy) cssObject.top = _top + "px";
$target.css(cssObject)
// gesturechange
$target.css({
width: _width + "px",
height: _height + "px",
webkitTransform: _rotate,
backgroundColor: "#4B880B"
});
Speaking of, $target is defined as $('#'+this.id), which only works for elements with an ID. A broader solution would be to set target using '$(this)':
$target = $(this);
You use have several obtuse checks around left and top. These can be slimmed down significantly.
// Before
var curLeft = ($('#' + _target).css("left") == 'auto') ? this.pageX : parseInt($('#' + _target).css("left"));
var _lastleft = (isNaN(parseInt($('#' + _target).css("left")))) ? 0 : parseInt($('#' + _target).css("left"));
_left = ($('#' + _target).css("left") == 'auto') ? this.pageX : parseInt($('#' + _target).css("left"));
// After
var curLeft = parseInt($target.css("left")) || this.pageX
var _lastLeft = parseInt($target.css("left")) || 0
_left = parseInt($target.css("left")) || this.pageX
Finally, you have several magic numbers. To make code maitenance easier, define variables for constants like colors, strings, and timeouts.
var END_COLOR = "#0B4188",
READY_COLOR = "#999",
MOVING_COLOR = "#4B880B";
var END_TIMEOUT = 5000;