0
\$\begingroup\$

I have the following code from a JavaScript file:

$("body").on("click",".edit",function(){
    var list=$(this).closest("a").data("list");
    var seira=$(this).closest("a").data("order");
    //alert(navbar_edit[list][seira]);
    var edit=navbar_edit[list][seira];
    navbar_edit[list][seira]=prompt("Change property",edit);
    exportLists();

});
$("body").on("click",".edit-save",function(){
    var property=$(this).closest("a").text();
    property=property.substring(0,property.length-4);
    alert(property);
    var list=$(this).closest(".list-group").attr("id");
    list=list.substring(list.length-1);
    navbar_edit[list].push(property);
    exportLists();
});

As you can see, I use $("body") 3 times to execute 3 different actions. Is there any way to use it only once so I can reduce my code?

\$\endgroup\$
  • \$\begingroup\$ I only see two $("body") and two click handlers. \$\endgroup\$ – 200_success May 20 '15 at 16:39
  • \$\begingroup\$ It's rather difficult to review just these two handlers in isolation without context. It would help if you also showed us a sample of the HTML and explained the goal that you are trying to achieve. Also, what does exportLists() do, and is it related to all this jQuery selection stuff? \$\endgroup\$ – 200_success May 20 '15 at 16:43
  • \$\begingroup\$ almost the whole html content is generated via jquery \$\endgroup\$ – cssGEEK May 20 '15 at 21:18
  • \$\begingroup\$ Can you just describe it then? \$\endgroup\$ – 200_success May 20 '15 at 21:40
1
\$\begingroup\$

Like many jQuery methods, .on() returns jQuery object which you can use for chaining purpose.

Sidenote: You should take into account that prompt method may return null if the user cancel dialog. Therefore you have to check the returning value before assigning it to the target variable (navbar_edit[list][seira]).

So the code may look like this:

$("body").on("click",".edit",function(){
    var list=$(this).closest("a").data("list");
    var seira=$(this).closest("a").data("order");
    //alert(navbar_edit[list][seira]);
    var edit=navbar_edit[list][seira];
    var v=prompt("Change property",edit);
    if (v) navbar_edit[list][seira]=v;
    exportLists();

}).on("click",".edit-save",function(){
    var property=$(this).closest("a").text();
    property=property.substring(0,property.length-4);
    alert(property);
    var list=$(this).closest(".list-group").attr("id");
    list=list.substring(list.length-1);
    navbar_edit[list].push(property);
    exportLists();
});
|improve this answer|||||
\$\endgroup\$
3
\$\begingroup\$

A few notes:

Always use .slice over .substring (Yes .slice works on strings.), it will support negative selection af strings, eg "abc".slice(0, -1) // "ab"
Storing reused values / objects in variables cleans up the code a lot, eg:

var a = $(this).find('.element').attr('a');
var b = $(this).find('.element').attr('b');
// The above can be written as: 
var $element = $(this).find('.element');
var a = $element.attr('a');
var b = $element.attr('b');
// This is faster and is IMO easier to read.

Meaningful variable names, dont use names like seira when you are referring to an index in an array call it index or list_index etc. (I assume its meaningful in another language but IMO all code should be written in english with english comments, and english variable names.)

Code:

$(document.body).on('click', '.edit', function() {
    var $a = $(this).closest('a');
    var list_id = $a.data('list');
    var index = $a.data('order');

    var current_value = navbar_edit[list][index];
    var new_value = prompt('Change Property', current_value);

    navbar_edit[list][index] = new_value;

    exportLists();
});
$(document.body).on('click', '.edit-save', function() {
    var $a = $(this).closest('a');
    var $listGroup = $(this).closest('.list-group');

    var value = $a.text().slice(0, -4);
    var list_id = $listGroup.attr('id').slice(0, -1); // remove last char

    navbar_edit[list_id].push(value);

    exportLists();
});
|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ well seira in greek is simillar to index.Thanks anyway \$\endgroup\$ – cssGEEK May 20 '15 at 15:38
  • \$\begingroup\$ All-English code makes more sense. However, if you are going to use Greek, why not write σειρά? That's a valid JavaScript identifier. \$\endgroup\$ – 200_success May 20 '15 at 16:48
  • \$\begingroup\$ I didn't know I could use Greek characters \$\endgroup\$ – cssGEEK May 20 '15 at 18:36

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.