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

Handling a dragging gesture using jQuery

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

Problem

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) {
_z

Solution

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.

Code Snippets

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;
_left = (e.pageX - curLeft);
_top = (e.pageY - curTop);
_dragging = [_left, _top];
$('#' + _target).css({
     backgroundColor: "#4B880B"
 });
 $('#' + _target + ' b').text('WEEEEEEEE!!!!');
$('#' + _target)
     .css({
         backgroundColor: "#4B880B"
     })
     .find('b')
     .text('WEEEEEEEE!!!!');
if (_dragx || _dragy) $('#' + _target).css({
    position: "absolute"
});
if (_dragx) $('#' + _target).css({
    left: _left + "px"
});
if (_dragy) $('#' + _target).css({
    top: _top + "px"
});

Context

StackExchange Code Review Q#15441, answer score: 8

Revisions (0)

No revisions yet.