Javascript app review

Question

So this is an app I'm working on, and I'd like to get some feedback on it. I'm leaving some key parts out, as I don't want everyone everywhere to have access to the complete code. The main pieces that I would like you guys to look at are still there though.

// Does a remote AJAX request that scrapes the URL and parses it
function doAjax(url) {
    $.getJSON(URL,

    function (data) {
        if (data.results[0]) {
            $("#content").html(""); // Reset
            var number = $(filterData(data.results[0])).find("#gtv_leftcolumn table:gt(1)");
            for (var i = 0; i < number.length; i++) {
                var name = $(filterData(data.results[0])).find("#gtv_leftcolumn table:gt(1) .maintext p:eq(" + i + ")").text();
                var type = $(filterData(data.results[0])).find("#gtv_leftcolumn table:gt(1) .trafficbriefs:nth-child(even) p:eq(" + i + ")").text();
                // Redacted
            }
            if (doAjax) {
                // Redacted
                if (number.length === 0) {
                // Checks to see if there are any elements on the page, and if 0, runs this
                    // Redacted
                } else {
                    checkFavorite();
                    var mySearch = $('input#id_search').quicksearch('#content .row', {
                        clearSearch: '#clearsearch'
                    });
                    mySearch.cache();
                    console.log("Loaded " + number.length);
                    console.log("Cached");
                }
            }
        } else {
            console.log("error");
        }
    });
}

function filterData(data) {
    data = data.replace(/<?\/body[^>]*>/g, '');
    data = data.replace(/[\r|\n]+/g, '');
    data = data.replace(/<--[\S\s]*?-->/g, '');
    data = data.replace(/<noscript[^>]*>[\S\s]*?<\/noscript>/g, '');
    data = data.replace(/<script[^>]*>[\S\s]*?<\/script>/g, '');
    data = data.replace(/<script.*\/>/, '');
    data = data.replace(/<img[^>]*>/g, '');
    return data;
}

// On Load
doAjax("http://www.codekraken.com/testing/snowday/wgrz.html");
$("#info").click(showInfo);
$(".info").click(closeInfo);
$("#reload").click(reaload);
$("#clearsearch").click(clearSearchBox);
$(".clear").click(clearFavorite);
setFavorite();

// You can clear the favorite item you set in setFavorite()
function clearFavorite() {
    localStorage.removeItem("favorite");
    localStorage.removeItem("favorite-status");
    $(".star-inside").removeClass("favorite");
    $(".clear span").text("");
}

// Clear search box
function clearSearchBox() {
    $("#id_search").val("");
    $('#id_search').trigger('keyup');
}

// Show info box
function showInfo() {
    // Redacted
}

// Close info box
function closeInfo() {
    // Redacted
}

// Reload AJAX request
function reload() {
    closeInfo();
    doAjax("URL");
}

// Set favorite item. This enables you to swipe on any .row element, and once it swipes, it sets the row you swipe on as the favorite. Swiping again unfavorites it. I mainly want help on this, as far as cleaning it up.
function setFavorite() {
    var threshold = {
        x: 30,
        y: 10
    };
    var originalCoord = {
        x: 0,
        y: 0
    };
    var finalCoord = {
        x: 0,
        y: 0
    };

    function touchMove() {
        console.log(event.targetTouches);
        finalCoord.x = event.targetTouches[0].pageX;
        changeX = originalCoord.x - finalCoord.x;
        var changeY = originalCoord.y - finalCoord.y;
        if (changeY < threshold.y && changeY > (threshold.y * -1)) {
            changeX = originalCoord.x - finalCoord.x;
            if (changeX > threshold.x) {
                window.removeEventListener('touchmove', touchMove, false);
                $(document).off("touchmove", ".row");
                if ($(event.target).attr("class") === "row-inside") {
                    var element = $(event.target);
                }
                if ($(event.target).attr("class") === "row-l") {
                    var element = $(event.target).parent();
                }
                if ($(event.target).attr("class") === "row-r") {
                    var element = $(event.target).parent();
                }
                var text = $(element).find(".row-l").text();
                var favstatus = $(element).find(".row-r").text();
                var thisStar = $(element).parent().find(".star-inside");
                $(element).css("margin-left", "-75px");
                if ($(thisStar).hasClass("favorite")) {
                    $(".clear span").text("");
                    $(thisStar).removeClass("favorite");
                    localStorage.removeItem("favorite");
                    localStorage.removeItem("favorite-status");
                } else {
                    $(".clear span").text("\"" + text + "\"");
                    localStorage.setItem("favorite", text);
                    localStorage.setItem("favorite-status", favstatus);
                    $(".star-inside").not(thisStar).removeClass("favorite");
                    $(thisStar).addClass("favorite");
                }
                setTimeout(function () {
                    $(element).css("margin-left", "0px");
                }, 500);
                setTimeout(function () {
                    $(document).on("touchmove", ".row", function () {
                        touchMove();
                    });
                }, 800);
            }
        }
    }

    function touchStart() {
        originalCoord.x = event.targetTouches[0].pageX;
        finalCoord.x = originalCoord.x;
    }
    $(document).on("touchmove", ".row", function () {
        touchMove();
    });
    $(document).on("touchstart", ".row", function () {
        touchStart();
    });
}

// Check favorite set in setFavorite()
function checkFavorite() {
    if (localStorage.getItem("favorite") !== null) {
        var name = localStorage.getItem("favorite");
        var favstatus = localStorage.getItem("favorite-status");
        var favstatusSplit = favstatus.substr(2);
        var favstatusLower = favstatusSplit.toLowerCase();
        var string = $(".row-l").text().toLowerCase();
        var re = new RegExp(name.toLowerCase(), 'g');
        var test = string.match(re);
        $(".row-l:contains(" + name + ")").parent().parent().find(".star-inside").addClass("favorite");
        if (test !== null) {
            $(".fav_school_inside").text(name + " - " + favstatusLower + "!");
            $(".clear span").text("\"" + name + "\"");
            setTimeout(function () {
                $(".fav_school").addClass("top");
            }, 1000);
            setTimeout(function () {
                $(".fav_school").removeClass("top");
            }, 6000);
        }
    }
}

It'd probably be beneficial if you wrote a few sentences about what the code is supposed to do and/or what you're particularly interested in having reviewed. Also, try running your code through jshint or jslint. Lastly, for the part about regex'ing HTML, see this question on SO (if nothing else, just because the answer's funny)
I love that SO question, but that's partially why I'm here: Is there a better way to do all of this? And I'll include some background for the code...

Flambino · Accepted Answer · 2012-10-25 03:07:13Z

Alright, figure I'll go ahead and say this right off the bat: I am no JS/JQuery guru. However, there were a few things that I saw that you might want to take a look at. This is by no means complete, but hopefully it will help.

You are violating the Arrow Anti-Pattern. This means your code is too heavily indented and should be refactored to remove some of that indentation. For example, the first if statement could be reversed and return early. This would then make an else statement unnecessary thus reducing a whole level of indentation from your entire function.

if( ! data.results[ 0 ] ) {
    console.log( 'error' );
    return;
}

//rest of code...

You can use empty() instead of explicitly clearing a field. This makes it a bit more obvious what you are trying to do wihtout needing comments everywhere trying to explain it.

$( '#content' ).empty();

Caching your selectors avoids having to compile them again and again, saving you processing power.

var $filterData = $( filterData( data.results[ 0 ] ) );
var number = $filterData.find( '#gtv_leftcolumn table:gt(1)' );

I don't know if this next suggestion will actually work, but it seems like it should. If it is possible to .find() from a .find(), in other words chaining finds, then I would try reusing your find results.

var name = number.find( ".maintext p:eq(" + i + ")" ).text();

I think you can get away with just loosley querying the length instead of explicitly. But that might just be preference.

if( ! number.length ) {

I don't like your filterData() function at all. First off, it seems like you should be able to refactor it to avoid redefining the same variable again and again. Second, I'm just not sure I see the point. To me it looks like this is just removing unwanted tags from something. No idea what, but it seems like you should be able to use a selector to chose what part of that document you want without having to regex it. Because I don't know exactly what you are trying to do here I can't make a good suggestion, sorry.

The following is violating the "Don't Repeat Yourself" (DRY) Principle. It should be refactored to avoid this. There may be other places that also violate this principle, but this is the first that popped out at me. The first thing that came to mind was to use a switch, but it quickly became apparent that once your abstract the first instance it really only had one other instance. This means you could have gotten away with a single if statement from the beginning. Another problem, not immediately obvious is that element is not always defined. Because you only define it in if statements there is a possibility that it may not be assigned and your script does not gracefully fail to compensate. Either declare a default value, or gracefully return to show an error has occurred. I went with the former.

//original code
if ($(event.target).attr("class") === "row-inside") {
    var element = $(event.target);
}
if ($(event.target).attr("class") === "row-l") {
    var element = $(event.target).parent();
}
if ($(event.target).attr("class") === "row-r") {
    var element = $(event.target).parent();
}

//refactored with default
//now with multiple uses
var element = $( event.target );
if( element.attr( 'class' ) !== 'row-inside' ) {
    element = element.parent();
}

I think its typically accepted that styling should only be done in CSS. I believe a better way would be to add and/or remove a class to get this same effect.

$(element).css("margin-left", "-75px");

Part of what I believe tucaz was saying was to do something like this:

$(document).on( {
    'touchmove' : function () {
        touchMove();
    },
    'touchstart' : function () {
        touchStart();
    }
}, '.row' );

There's more to his answer, I'm sure, this is just the only part I'm sure of. The above creates an event object that can easily be added to. It also has the added benefit of grouping related functionality. Something you might want to consider is adding namespaces to your events. They already seem to have a pseudo-namespace, but you might want to explicitly declare it. This helps differentiate your custom events from normal ones, while also allowing you to keep track of their purpose.

'touch:move' : function() {

This last section is going to be a general blanket statement. You have too few functions for too much functionality. This violates the Single Responsibility Principle. In other words, functions should only do one thing, everything else should be delegated to another function. The functions you do have are bulky and difficult to follow because of this. Adding more functions should help you to make your code more legible and will make it easier to extend this later. This is also key in ensuring your code does not violate DRY.

While I'm sure there are things I missed, I hope some of this helps.

Just to assuage your doubt: Yup, you can chain find() calls all you want. jQuery basically lets you chain any and all of the element selection functions
@Flambino: Thank you for clearing that up. I meant to try that out at some point yesterday but never got around to it.

tucaz · Answer 2 · 2012-10-23 11:34:38Z

This is not a complete review but one concerning organization aspects only.

The first thing I suggest you do is organize your javascript into objects and classes such. Having a bunch of functions lying around usually is not the best idea as they are really hard to test and mantain after you grow to a few hundred functions. Organizing functions into coherent simple objects is a much better way to deal with javascript application growing.

This series of talks by Douglas Crockford[1] provide an invaluable amount of good stuff regarding javascript.

Also, I can see that you have a lot of DOM manipulation mixed with your app logic. Another good idea is to separate it by using a MVVM framework like Knockout JS[2] where you can leverage it´s binding capabilities leaving the DOM manipulation totally away from your objects.

If you do not want to use a framework like that I suggest at least you use a template engine like handlebars or mustache to deal with HTML building.

YUI talks by Douglas Crockford -> http://yuiblog.com/crockford/
Knockout JS including live tutorial -> http://learn.knockoutjs.com/

asked	8 months ago
viewed	246 times
active	8 months ago

Javascript app review

2 Answers

Your Answer

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

Javascript app review

2 Answers

Your Answer

Sign up or log in

Post as a guest

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

Related