Can this Drag and Drop Script be Optimized to Perform Better?

Question

I've just finished a week long adventure of creating an HTML5 Drag and Drop scheduler. As the prototype stands it works fine, but I'm curious if some of the code can be optimized. I mostly focused on it being readable and well documented, but I'm curious if someone can come up with any optimizations.

I've noticed that when Firefox is used there is some "twitching" that occurs when the cells are updated. Here's the code (minus the comments that are available on the fiddle):

"use strict";

var unscheduledTbody = null,
    scheduledTbody = null,
    dataTransferValue = null,
    isDroppable = true;

var getIndicesOf = function (td) {
    var tr = td.parent(),
        tbody = tr.parent();

    var row = tbody.children().index(tr),
        column = tr.children().index(td);

    return [row, column];
};

var verifyRowspan = function (options) {
    var tdIndicies = getIndicesOf(options.td),
        selectors = "",
        elements = null,
        element = null,
        i = 0;

    for (; i < options.rows; i++) {
        selectors = (selectors + ("tr:eq(" + (tdIndicies[0] + i) + ") td." + tdIndicies[1] + ","));
    }

    selectors = selectors.substring(0, (selectors.length - 1));
    elements = scheduledTbody.find(selectors);

    for (i = 0; i < elements.length; i++) {
        element = elements[i];

        if (element.id !== "") {
            return false;
        }
    }

    tdIndicies = null;
    selectors = null;
    elements = null;
    element = null;
    i = null;

    return true;
};

var toggleVisibility = function (options) {
    var tdIndicies = getIndicesOf(options.td),
        selectors = "",
        i = 1;

    for (; i < options.rows; i++) {
        if (options.hide) {
            selectors = (selectors + ("tr:eq(" + (tdIndicies[0] + i) + ") td." + tdIndicies[1] + ","));
        } else {
            selectors = (selectors + ("tr:eq(" + (tdIndicies[0] + i) + ") td." + tdIndicies[1] + ":hidden,"));
        }
    }

    selectors = selectors.substring(0, (selectors.length - 1));

    if (selectors.length > 0) {
        scheduledTbody.find(selectors).css({
            display: (options.hide ? "none" : "table-cell")
        });
    }

    options = null;
    tdIndicies = null;
    i = null;
};

var dragEnterHandler = function (e) {
    dataTransferValue = e.originalEvent.dataTransfer.getData("text/plain");

    var source = $("#" + dataTransferValue),
        target = $(this),
        rows = source.data("rows");

    isDroppable = verifyRowspan({
        td: target,
        rows: rows
    });

    if (isDroppable) {
        e.originalEvent.dataTransfer.dropEffect = "move";

        $(this).addClass("Droppable");
    } else {
        $(this).addClass("NotDroppable");
    }

    source = null;
    target = null;
    rows = null;
};

var dropHandler = function (e) {
    e.preventDefault();

    var target = $(this);

    if (isDroppable) {
        var source = $("#" + dataTransferValue),
            rows = source.data("rows"),
            url = source.data("url");

        target.removeClass("Droppable")
            .removeClass("NotDroppable")
            .html(source.html())
            .attr("id", source.attr("id"))
            .attr("rowspan", rows)
            .data("rows", rows)
            .data("url", url);

        source.html("")
            .removeAttr("id")
            .removeAttr("rowspan")
            .removeData("rows")
            .removeData("url");

        toggleVisibility({
            td: target,
            rows: rows,
            hide: true
        });

        source = null;
        rows = null;
        url = null;
    } else {
        $("b").text("The cell could not be dropped at the target location. It conflicted with an existing cell in its path.");

        target.removeClass("Droppable").removeClass("NotDroppable");
    }

    target = null;
};

$(function () {
    if (console.clear) {
        console.clear();
    }

    unscheduledTbody = $("#Unscheduled tbody");
    scheduledTbody = $("#Scheduled tbody");

    $("#Unscheduled tbody,#Scheduled tbody").on({
        dragstart:function (e) {
            e.originalEvent.dataTransfer.effectAllowed = "move";
            e.originalEvent.dataTransfer.setData("text/plain", this.id);
        },
        dragend: function (e) {
            switch (e.originalEvent.dataTransfer.dropEffect) {
                case "copy":
                case "move":
                    if (isDroppable) {
                        var source = $(this),
                            target = $("#" + dataTransferValue);

                        if (source.data("scheduled")) {
                            var rows = target.data("rows");

                            toggleVisibility({
                                td: source,
                                rows: rows,
                                hide: false
                            });

                            rows = null;
                        } else {
                            source.parent().remove();
                        }

                        dataTransferValue = null;
                        isDroppable = true;
                        source = null;
                        target = null;
                    }

                    break;
                case "link":
                case "none":
                    break;
                default:
                    break;
            }
        }
    }, "[draggable]");

    scheduledTbody.on({
        dragenter: dragEnterHandler,
        dragover: function (e) {
            e.preventDefault();
        },
        dragleave: function (e) {
            $(this)
                .removeClass("Droppable")
                .removeClass("NotDroppable");
        },
        drop: dropHandler
    }, "td").on("click", "a", function (e) {
        e.preventDefault();

        var source = $(this).closest("td"),
            rows = source.data("rows"),
            url = source.data("url");

        unscheduledTbody
            .append($("<tr />")
                .append($("<td draggable='true' data-scheduled='false' />")
                    .html(source.html())
                    .attr("id", source.attr("id"))
                    .data("rows", rows)
                    .data("url", url)));

        source.html("")
            .removeAttr("id")
            .removeAttr("rowspan")
            .removeData("rows")
            .removeData("url");

        toggleVisibility({
            td: source,
            rows: rows,
            hide: false
        });

        source = null;
        rows = null;
        url = null;
    });
});

UPDATE

I spent most of the day today trying to get @ANeves' suggestions implemented.

1. I've replaced any double quote escaping.
2. I've aded use strict;.
3. I've replaced the == comparer with ===.
4. I renamed Indecies to getIndicesOf (I opted to keep it as indices instead of indexes because it just didn't sound right to me).
5. All functions were renamed and start with lower case letters.
6. Converted everything from using .live() to .on(), which was an interesting experience. Thanks for pointing me to .on() @ANeves, I didn't even know about its existence.

I've also shuffled a huge chunk of code out into reusable functions, especially the ones that deal with manipulating the cells.

I tried not to pollute the global namespace, but I just couldn't find a way around it. If someone has suggestions on getting that done, please by all means let me know.

As it stands, the prototype is mostly usable. One bug I can't seem to get a grip on is if I want to move a cell right above itself. For example, if you look at the JSFiddle, if you place Item D anywhere and then try to move it once cell up, it hits the verifyRowspan check and cancels out because it conflicts with itself. I tried getting around it by also checking if one of the cells in it's path has the id that it does, but it doesn't work very reliably. I'd appreciate suggestions on this.

And lastly, if any of you see anything else that's wrong or could be improved, please let me know.

Thanks!

Answer 1

I don't really address the concrete questions you made; I hope someone else does. :)

---

As the prototype stands it works fine

Famous last words! You have at least two bugs:

1. Drag D on top of B, and B is lost forever.
2. Now drag A on top of D, and not only D disappears but the layout is ruined.

You will get some good tips by running your code through JSLint.
Some quick and non-exaustive remarks:

• You can avoid escaping quotes by alternating between single and double quotes, which both JavaScript and HTML use interchangeably: use str = "<foo bar='nix' />" instead of str = "<foo bar=\"nix\" />";
• Do not pollute the global namespace: encapsulate the code in a self-executing function;
• Starting the code with "use strict"; will really help with identifying many problems early;
• Use === to compare, and not ==;
• indecies should be called indexes (actually, getIndexesOf);
• Constructors start with a capital letter - all those methods are not constructors, so they should not start with a capital letter;
• Use .on('event', ...) instead of live, and maybe even use the .on overload that takes an "event-map".

Note that .on will not bind to objects that are later loaded into the DOM. But you can bind to a parent element, and provide a selector to filter which children elements to catch the event on. This not only catches events on such children that might be later attached (same as live), but also attaches a single event handler instead of a million. Take heed not to use document to attach these, or similar elements high in the DOM tree - read the API: http://api.jquery.com/on/