6
\$\begingroup\$

Why this plugin:

I am developing a mobile app - at some point, I felt like this would be a good idea to give the users the possibility to control everything in the app with touch gestures, hence the need for a plugin able to recognize more than the basic swipe events.

What it does:

When the user starts touching the screen and moving his finger (or mouse on desktop btw), the code tries to identify his move as one of the eight cardinal directions (North, South, West, East, NorthWest, SouthWest, SouthEast, NorthEast). If he changes direction, then the next direction he takes is added to the list to produce an output like : North->East->South...

Why I am posting here:

This code is going to be run quite a lot of times in the app (every time the user moves his finger on the screen), and I would love to be sure that it is not going to pose performance issues, or if I overlooked some best practices I am not aware of, and of course, any constructive feedback is very welcome.

The part of the code to attach the event and handle touchStart and touchStop is not mine, it is taken from here, but I am including it for plugin completeness (also modified some very minor things).

The code:

// jquerymobile-unicorn_swipe
// ----------------------------------
// Copyright (c)2012 Donnovan Lewis
// Copyright (c)2014 Romain Le Bail
// Distributed under MIT license (http://opensource.org/licenses/MIT)
//
//credits to Donnovan Lewis for the material taken from https://github.com/blackdynamo/jquerymobile-swipeupdown

(function () { 
// initializes touch events
    var supportTouch = $.support.touch,
        touchStartEvent = supportTouch ? "touchstart" : "mousedown",
        touchStopEvent = supportTouch ? "touchend" : "mouseup",
        touchMoveEvent = supportTouch ? "touchmove" : "mousemove";
$.event.special.unicorn = { 
    setup: function () {
        var thisObject = this;
        var $this = $(thisObject);

        $this.bind(touchStartEvent, function (event) {   
            var path = []; var derived_path = []; var segs = [[]]; var seg_types =[[]]; var max_freqs = [[]];
            var data = event.originalEvent.touches ?
                    event.originalEvent.touches[ 0 ] :
                    event,
                start = {
                    time: (new Date).getTime(),
                    coords: [ data.pageX, data.pageY ],
                    origin: $(event.target)
                },
                stop = false;
            var n = 0;
            path[n]=[ data.pageX, data.pageY ]; /* starting point */

            function moveHandler(event) {
                if (!start) {
                    return;
                }
                var data = event.originalEvent.touches ?
                    event.originalEvent.touches[ 0 ] :
                    event;
                stop = {
                    time: (new Date).getTime(),
                    coords: [ data.pageX, data.pageY ]
                };
                n++;
                path[n]=[ data.pageX, data.pageY ];
                // prevent scrolling
                //if (Math.abs(start.coords[1] - stop.coords[1]) > 10) {event.preventDefault();}
            }

            $this
                .bind(touchMoveEvent, moveHandler)
                .one(touchStopEvent, function (event) {  
                    $this.unbind(touchMoveEvent, moveHandler);    /* now we stopped moving */
                    var l = path.length; 
                    var min_length = 8; /* min length is to have enough points to perform consistent recognition */ 
                    var fire = false;

                    if(l>min_length && stop.time - start.time < 10000) 
                    {

                    for(var i=1;i<l-1;++i)
                    {
                        var d_i = (path[i+1][1]-path[i-1][1]) / (path[i+1][0]-path[i-1][0]); /* maybe use x'(t) and y'(t) in addition to y'(x)*/
                        var x_sig_i = path[i+1][0]-path[i-1][0]<0 ? -1 : 1; 
                        derived_path[i-1] = d_i==Infinity ? 1000 : d_i==-Infinity ? -1000 : d_i>3 ? 1000*x_sig_i : d_i<-3 ? -1000*x_sig_i /* vertical moves */
                                : d_i>=-2 && d_i<=-0.5 ? -1 : d_i>=0.5 && d_i<=2 ? 1 /* diagonal moves */
                                : Math.abs(d_i)<=0.5 ? 0 : d_i; /* horizontal moves */
                    };

                    for(var i=0;i<=l-min_length;++i) /* create sub paths of min_length-2 points */
                    {
                        segs[i]=derived_path.slice(i,i+min_length-2);
                        seg_types[i]=[0,0];
                        if(path[i+7][1]<path[i][1]){seg_types[i][1]=1;}; /* four quadrants */
                        if(path[i+7][1]>path[i][1]){seg_types[i][1]=-1;};
                        if(path[i+7][0]>path[i][0]){seg_types[i][0]=1;};
                        if(path[i+7][0]<path[i][0]){seg_types[i][0]=-1;};
                    }
                    for(var i=0;i<=l-min_length;++i)
                    { 
                        segs[i].sort(function(a,b){return a - b;});  /* sorting to count duplicates */
                        var previous = segs[i][0];
                        var popular = segs[i][0];
                        var count = 1;
                        var max_count = 1;

                        for (var j=1; j<8; j++) 
                        { 
                            if (segs[i][j] == previous)
                            {count++;}
                            else 
                            {
                                if (count > max_count) 
                                {
                                    popular = segs[i][j-1];
                                    max_count = count;
                                };
                                previous = segs[i][j];
                                count = 1;
                            };
                        var pop = count > max_count ? segs[i][min_length-3] : popular;
                        var cnt = count > max_count ? count : max_count;
                        max_freqs[i]=[pop,cnt];
                        }

                    }

                    for(var i=max_freqs.length-1; i>=0; i--)
                    {    
                        if(max_freqs[i][1]<=4){max_freqs.splice(i,1);seg_types.splice(i,1);};  /* eliminate sub paths without clear direction */
                    }
                    var previous = max_freqs[max_freqs.length-1];
                    var previous_type = seg_types[max_freqs.length-1];
                    for(var i=max_freqs.length-2; i>=0; i--)
                    {    
                        if(previous[0]==max_freqs[i][0] && (previous_type[0]==seg_types[i][0] && previous_type[1]==seg_types[i][1] /* merge consecutive sub paths */
                            || Math.abs(max_freqs[i][0])>500)
                            || (max_freqs[i][0]==0 && previous_type[0]==seg_types[i][0]))
                        {
                            if(previous[1]>max_freqs[i][1]){max_freqs.splice(i,1);seg_types.splice(i,1);}
                            else{max_freqs.splice(i+1,1);seg_types.splice(i,1);};
                        }
                        previous = max_freqs[i];
                        previous_type = seg_types[i];
                    }

                    var typezor="";
                    for(var i=0;i<max_freqs.length;++i)
                    {
                        var p_i = max_freqs[i][0];
                        var t_x_i = seg_types[i][0];
                        var t_y_i = seg_types[i][1];
                        if(i>0){typezor += "->";};
                        typezor += p_i==-1000 ? "North" : p_i==1000 ? "South" : p_i==0 ? (t_x_i > 0 ? "East" : "West") : 
                            p_i == -1 ? (t_y_i > 0 ? "NorthEast" : "SouthWest") : (t_y_i > 0 ? "NorthWest" : "SouthEast");
                    }
                    if(i!=0){fire=true;};
                    }
                    if (start && stop) {
                        if (fire) {  
                            start.origin.trigger({type:"unicorn",typezor:typezor});

                        }
                    }
                    start = stop = false;
                });
        });
    }
};


})();

How to test:

If anyone finds this funny, you can test it on desktop PC by adding an event handler:

$(document).on('unicorn', function(event, ui){ console.warn(event.typezor); });

or simply by replacing the start.origin.trigger({type:"unicorn",typezor:typezor}); line by an alert/console.log.

\$\endgroup\$
3
\$\begingroup\$

Interesting plugin,

some observations:

  • Use JsHint, there are a ton of minor issues with this code that you should clean up
  • Use JsBeautifier, there is quite a bit of inconsistent formatting
  • 'unicorn' sounds like fun, 'typezor' not so much
  • It seems this code does not handle simultaneous touch events ?
  • From a style perspective,

    • Too much horizontal stretching, why ?

      var path = []; var derived_path = []; var segs = [[]]; var seg_types =[[]]; var max_freqs = [[]];
      
    • Naming is often too cryptic, plus JavaScript should use lowerCamelCase ( max_count -> maxCount )
    • Nested ternary operators are too hackish
    • if(i!=0){fire=true;}; could be fire = fire || !i; though I might have considered that too hackish if you had actually written that ;)
    • Commenting : simply stuff is commented like /* starting point */ but the harder part ( for me ) where you derive a direction does not have enough comments

All in all, I think this code needs some more polishing before I would use it in a project.

\$\endgroup\$
  • \$\begingroup\$ Thanks for your feedback; as for naming issues, I often write code in a rush because I have a lot of things to do so I just use whatever comes first to my mind as a variable name; I will try to adopt a coherent naming system though. All your points are nice. \$\endgroup\$ – IazertyuiopI Jul 10 '14 at 4:40
  • \$\begingroup\$ The only thing I think I should not change is horizontal stretching; when a line of code is really clear to me and I know what it does from first sight (like here, init all arrays), I like to keep it a line to save space for the more complicated bits. \$\endgroup\$ – IazertyuiopI Jul 10 '14 at 4:41
  • \$\begingroup\$ Thanks :) Remember that Code Review is also about making sure that other people besides yourself can easily grok your code and potentially maintain it. \$\endgroup\$ – konijn Jul 10 '14 at 13:01
3
\$\begingroup\$

Architecture

Right away, something jumps out at me about the overall architecture. You have written this as a jQuery plugin, but it hardly relies on jQuery at all. On the user's end of things, it has nothing to do with jQuery (sure, you can use jQuery to set up your event listener, but you could do that with any custom event) and the places where it does rely on jQuery internally are fairly trivial and easily replaced with vanilla JS; for example:

supportTouch = $.support.touch

...can simply be:

supportTouch = "ontouchstart" in window

I think this would be nicer as a self-sufficient library, not just because you won't be forcing other dependencies on your users, but because it will (hopefully) encourage you to think of it as a first-class library which should promote a cleaner code base. Sounds crazy? Try it.

As a proof-of-concept, I've removed the jQuery dependencies. With this revision, your usage example still works exactly as before, but since this is now a native custom event, you can also do this:

addEventListener('unicorn', function (event) { console.log(event.traj); });

Naming and API

Konjin already addressed some of this, but I think the most important thing is to have the public API named sensibly. What the heck is a traj? If it's a trajectory, call it that. I'd rather type six more characters than look at half a word and wonder what it means. Maybe a better name would be gesture. The English language has a large vocabulary; find a nice descriptive word and use it. Internal identifiers are not quite as important, but don't skimp on the user-facing stuff.

Also, the way the data in traj is presented could probably be improved. I might have gone for an array containing abbreviations like ["SE", "NW"]. If the user wanted to compare it to a string, it would automatically convert to "SE,NW", but if the user wants it as an array, they don't have to do an awkward traj.split("->").

Complexity

It's hard to follow what's going on here, because you have very few named functions, and your functions, object literals, conditionals, loops and so on keep nesting deeper and deeper. Try to extract some things out into named functions, keeping them under around 25 lines each. Ideally someone reading your code should be able to look at an entire function in approximately 80 columns by 25 lines without scrolling, so he can easily comprehend what that function does and move on to the next. Your touchend handler could benefit from this.


Overall, this project has potential and I hope you keep improving it. Looking forward to a followup question and unicorn 2.0!

\$\endgroup\$
  • \$\begingroup\$ Thanks for that review and the w/o jQuery version; the reason I did not write it in vanilla js in the first place is quite simple : I have very limited knowledge of vanilla js =P But thanks to your proof on concept, there is no reason for the next version to be jquery plugin. By the way, when I will have made consequent changes, should I edit this post with a link to the new version or post a new question? \$\endgroup\$ – IazertyuiopI Jul 10 '14 at 9:43
  • \$\begingroup\$ You can post it as a new question if you want it reviewed again, and update this one with a link to the new one (but don't change any of the code here). There's a discussion about followups here. \$\endgroup\$ – Dagg Jul 10 '14 at 10:18
  • \$\begingroup\$ Well, the whole thing works properly without jquery now, so I put it on GitHub \$\endgroup\$ – IazertyuiopI Jul 11 '14 at 12:11
  • \$\begingroup\$ Looks good. I like route a lot better than traj. At this point the main thing I would suggest is moving all of the "STEP X" sections into separate functions. \$\endgroup\$ – Dagg Jul 11 '14 at 19:08
  • \$\begingroup\$ Actually, I tried that but rolled back afterwards because I thought people are going to read the function code anyways.. Should I define the function then call it or call all the functions one after another and define them at the end of the file? \$\endgroup\$ – IazertyuiopI Jul 14 '14 at 7:41

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.