2
\$\begingroup\$

I created a script to allow a user to drag a div around the page and save it's position and style to localStorage, and I'm wondering if there's anything I can do to make the script more efficient. I have posted the code below, but I left out the drag functionality because I'd like to work on that myself, but the rest I'd like feedback on.

$(document).ready(function () {
    // Load function (10 steps)
    function loadDiv() {
        $(".base").each(function () { // (1) Find each div with the class of base
            var id = $(this).attr("id"); // (2) Get the id from the element and set it to a variable
            var counter = localStorage.getItem("counter-" + id) || 0; // (3) See if counter is at 0 and get each LS key with the syntax (counter-(2))
            var active = localStorage.getItem(id + "-active") || ""; // (4) Get each LS key with the syntax ((2)-active) and see if it's undefined
            $.each(active.split(" "), function (k, v) {
                var s = v.split(","); // (5) Split each variable from (4) with K = Key and V = Value
                if (s.length != 2) { // (6) Check if length is not equal to 2
                    return; // (7) Do nothing
                }
                var newElement = $("#" + s[0]).clone(); // (8) Set the element that's getting cloned to a variable
                newElement.attr("id", s[1]).attr("class", "drag " + id).data("id", id).appendTo("body"); // (9) Clone element
            });
        });
        dragWidget(); // (10) Initialize dragging for these elements
    }
    // Set CSS function (5 steps)
    function setCSS() {
        $(".drag").each(function () { // (1) Find all divs on the page with the class drag
            var id = $(this).not(".base").attr("id"); // (2) Set then id of each div that's not .base to a variable
            $(this).css({
                "left": localStorage.getItem(id + "-x"), // (3) Retrieve localStorage item with the variable from (2) and append -x to the end to get and set left offset
                "top": localStorage.getItem(id + "-y"), // (4) Retrieve localStorage item with the variable from (2) and append -y to the end to get and set top offset
                "z-index": localStorage.getItem(id + "-z"), // (5) Retrieve localStorage item with the variable from (2) and append -z to the end to get and set the z-index
            });
        });
    }
    // Close function (7 steps)
    function closeDiv() {
        var id = $(this).parent().attr("id").match(/[a-zA-Z]+/g); // (1) Get only the first word from the parent element and assign it to a variable
        $(this).parent().remove(); // (2) Remove the parent element
        var active = []; // (3) Set the active variable to a blank array
        $($("." + id).not(".base")).each(function () { // (4) Go through every element that's not .base
            active.push(id + "," + $(this).attr("id")); // (5) Add each element to an array with the syntax ((1),(1)+id from element)
        });
        active = active.join(" "); // (6) Join the array together by a space
        localStorage.setItem(id + "-active", active); // (7) Set the LS item with the syntax ((1)-active, (5))
    }
    // Clone function (13 steps)
    function cloneDiv() {
        var id = $(this).attr("href").match(/[a-zA-Z]+/g); // (1) Get only the first word from the parent element and assign it to a variable
        var key = "counter-" + id; // (2) Set a variable with the syntax "counter-(1)"
        var counter = localStorage.getItem(key) || 0; // (3) Get the LS item with the name (2) and see if it's equal to 0
        counter++; // (4) Add one to the counter
        var newElement = $("#" + id).clone(); // (5) Set clone parameters to a variable
        newElement.attr("id", id + counter).attr("class", "drag " + id).appendTo("body"); // (6) Duplicate the div to the body
        var active = []; // (7) Set the active variable to an array
        $($("." + id).not(".base")).each(function () { // (8) Get all elements that match ".(1)" that aren't ".base" and loop through them
            active.push(id + ',' + $(this).attr("id")); // (9) Add each element to an array with the syntax ((1),(1)+id from element)
        });
        active = active.join(" "); // (10) Join the array together by a space
        localStorage.setItem(id + "-active", active); // (11) Set the LS item with the syntax ((1)-active, (7))
        localStorage.setItem(key, counter); // (12) Set the LS item with the syntax ((2), (4))
        dragWidget(); // (13) Enable dragging
    }
    // Clear localStorage (2 steps)
    function clearLocalstorage() {
        localStorage.clear(); // (1) Clear all localStorage items
        location.reload(); // (2) Reload the page
    }
    loadDiv();
    setCSS();
    $(".close").click(closeDiv);
    $(".nav a").click(cloneDiv);
    $(".clear a").click(clearLocalstorage);
});
\$\endgroup\$
3
\$\begingroup\$

Looks ok so far. Here are some tips.

1)

Only provide comments for sections arounds complex code. Also, it's best to place comment directly above the statements and not side by side.

Bad:

$(".base").each(function () { // (1) Find each div with the class of base
    var id = $(this).attr("id"); // (2) Get the id from the element and set it to a variable

Better:

// (1) Find each div with the class of base
$(".base").each(function () { 
    // (2) Get the id from the element and set it to a variable
    var id = $(this).attr("id"); 

Best:

$(".base").each(function () { 
    var id = $(this).attr("id");        

2)

Not really needed but I think you should create a function for creating the active string.

Old Code:

//...
var active = [];
$($("." + id).not(".base")).each(function () {
    active.push(id + "," + $(this).attr("id"));
});
active = active.join(" ");

//...
var active = [];
$($("." + id).not(".base")).each(function () {
    active.push(id + "," + $(this).attr("id"));
});
active = active.join(" ");
//...

New Code:

var getActiveIdStr = function( id ){
    var active = [];
    $($("." + id).not(".base")).each(function () {
        active.push(id + ',' + $(this).attr("id"));
    });
    return active.join(" ");
}
//...
getActiveIdStr( id );
//...
getActiveIdStr( id );

3)

Here's a tip on how to store objects to localStorage. The trick is to use JSON.stringify to set values and JSON.parse to retrieve values. More here:

Code:

var lsHelper = {};
lsHelper.set = function (key, value) {
    if (typeof value === "object") {
        value = JSON.stringify(value);
    }
    localStorage.setItem(key, value);
};
lsHelper.get = function (key) {
    var value = localStorage.getItem(key);
    if (value && value[0] === "{") {
        value = JSON.parse(value);
    }
    return value;
};

4)

The easiest way to save the positions of draggable elements is to use localStorage and the module "Draggable" from the jQuery UI project. Use lsHelper to save the position once an element is dropped. Then when the page is refreshed, use the create option in Draggable to re-position all the draggable elements by the positions saved in localStorage.

HTML Code:

<!DOCTYPE html>
<html>
<head>
  <link href="http://ajax.googleapis.com/ajax/libs/jqueryui/1.8/themes/base/jquery-ui.css" rel="stylesheet" type="text/css"/>

        Load using Script Tag: "http://ajax.googleapis.com/ajax/libs/jquery/1.5/jquery.min.js"
        Load using Script Tag: "http://ajax.googleapis.com/ajax/libs/jqueryui/1.8/jquery-ui.min.js"

  <style type="text/css">
  .dragMe { width: 100px; height: 70px; background: silver; }
  </style>
</head>
<body style="font-size:62.5%;">

<div id="draggable1" class="dragMe">1: Drag me</div>
<div id="draggable2" class="dragMe">2: Drag me</div>

</body>
</html>

JS Code:

var ls = ls || {};
ls.set = function (key, value) {
    if (typeof value === "object") {
        value = JSON.stringify(value);
    }
    localStorage.setItem(key, value);
};
ls.get = function (key) {
    var value = localStorage.getItem(key);
    if (value && value[0] === "{") {
        value = JSON.parse(value);
    }
    return value;
};

$(function () {
    $(".dragMe").draggable({
        create : function (event, ui) {
            var position = ls.get("position:" + $(this).attr("id"));
            if(position){
                $(this).offset( position );
            }
        },
        stop : function (event, ui) {             
            var currentPos = ui.helper.position();
            ls.set("position:" + $(this).attr("id"), currentPos);
        }
    });
});

Demo here: http://jsfiddle.net/DLM3n/

\$\endgroup\$
  • \$\begingroup\$ Do you think using regex is fine though? \$\endgroup\$ – Charlie Sep 16 '12 at 16:11
  • \$\begingroup\$ Yeah it's ok, but in cloneDiv() you should use $(this).parent().attr("href") instead of $(this).attr("href") to get the parent element. \$\endgroup\$ – Larry Battle Sep 17 '12 at 3:44

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.