How can i optimize this jquery code

Question

Is there a shorter more optimized way to write the following code.

$("button.idiv").click(function(){
        var elm = $('<div id=divid' + divId + ' class=aaa></div>');
        elm.resizable().draggable({ containment: "p", stack:"div" }).appendTo('p');
        divId++;
});

$("button.ispan").click(function(){
        var elm = $('<img id=spanid' + spanId + ' class=aaas src="Dollar.png"/>');
        elm.resizable().parent().draggable({ containment: "p", stack:"div" }).appendTo('p');
        spanId++;
});

$("button.itext").click(function(){
        var elm = $('<div id=textid' + textId + ' class=aaat>some text</div>');
        elm.resizable().draggable({ containment: "p", stack:"div" }).appendTo('p');
        textId++;
});

Is it running slowly? Do you have output from a profiler that shows where the bottleneck is? — Rob
– Rob, Commented Jan 8, 2011 at 21:31
No it's not running slow, But i find my self repeating and i want to know if i can write this code in a shorter more optimized way. I still have 10 more button clicks functions to call in addition to the 3 i already have in the code above. — Hussein
– Hussein, Commented Jan 8, 2011 at 21:35
@alex So you're not looking for an "optimization". You're looking for a way to keep code DRY (Don't Repeat Yourself). Lots of people (me included) dislike premature optimizations :p — user166390
– user166390, Commented Jan 8, 2011 at 21:49
@alex - it would help if we can see the structure of the markup. — redsquare
– redsquare, Commented Jan 8, 2011 at 22:01

David Tang · Accepted Answer · 2011-01-09 23:45:08Z

1

Since all three sets of code do almost the same thing except for some individual values, this is the perfect opportunity to consolidate the differences with some sort of configuration object:

var types = {
    div: {
        id: 1,
        element: "<div>",
        className: "aaa"
    },
    span: {
        id: 1,
        element: "<img src='Dollar.png'>",
        className: "aaas",
        parentDraggable: true
    },
    text: {
        id: 1,
        element: "<div>",
        className: "aaat"
    }
};

Then the code simply becomes:

$("button").click(function(){
    var $this = $(this), t;
    // See if the button has any class matching one of the defined "types"
    for (var i in types) {
        if (types.hasOwnProperty(i) && $this.hasClass("i" + i)) {
            t = i;
            break;
        }
    }
    if (!t) return;

    // Your condensed code
    var elem = $(types[t].element)
        .attr("id", t + "id").addClass(types[t].className)
        .resizable();
    if (types[t].parentDraggable) elem = elem.parent();
    elem.draggable({ containment: "p", stack:"div" }).appendTo('p');

    // Increment ID on the type itself
    types[t].id++;
});

edited Jan 9, 2011 at 23:45

answered Jan 9, 2011 at 11:07

David Tang

93.8k32 gold badges169 silver badges149 bronze badges

Sign up to request clarification or add additional context in comments.

4 Comments

mekwall Over a year ago

Nice, but it doesn't mirror the behavior of his code. There's more to it then just different class names and elements. Notice the .parent() that's set to draggable for .ispan and src for the img element. Also, your example will select all button elements which is far from optimal.

David Tang Over a year ago

@Marcus, I overlooked these, thanks for pointing them out. I'll modify the code to suit. Attaching the click to all button elements is less efficient, but more flexible when you need to add new "types". If you look at the poster's comments, he actually wants 10 of these but in the example only showed 3. Listing each class as part of the selector is quite tedious and error-prone.

mekwall Over a year ago

cool! Well, perhaps we should have a second class to tell which one to select then. That's how I would do it atleast :)

David Tang Over a year ago

@Marcus, I absolutely agree. That's something for @alex to decide then.

mekwall · Accepted Answer · 2011-01-09 14:20:26Z

0

Are you looking to do something like this? The code might look like more, but I've tried to remove as much of the repeating as possible and also added lots of newlines and tabs to make it easier to read. Any difference in performance should be neglible.

$("button.idiv, button.itext, button.ispan").click(function(){
   var $this = $(this),
       draggableOptions = {
               containment: "p", 
               stack:"div"
       };
   if ($this.is(".idiv, .itext")) {
       var elm = $("<div>"),
           cls = "aaa";
       if ($this.is(".idiv")) {
           id = "divid"+divId;
           divId++;
       } else {
           id = "textid"+textId;
           cls += "t";
           textId++;
       }
       elm
           .addClass(cls)
           .attr("id", id)
           .resizable();
   } else {
       var elm = $("<img>")
           .attr("id", "spanid"+spanId)
           .attr("src", "Dollar.png")
           .addClass(cls+"s")
           .resizable();
       elm = elm.parent();
       spanId++;
   }
   elm.draggable(draggableOptions)
      .appendTo("p");
});

edited Jan 9, 2011 at 14:20

answered Jan 9, 2011 at 10:14

mekwall

29.1k6 gold badges78 silver badges77 bronze badges

1 Comment

Hussein Over a year ago

Very clean. Thanks for your input.

Collectives™ on Stack Overflow

How can i optimize this jquery code

2 Answers 2

4 Comments

1 Comment

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

4 Comments

1 Comment

Your Answer

Sign up or log in

Post as a guest

Linked

Related