Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 1 down vote favorite

I've just completed a Javascript module and I've got a strong feeling that this is borderline spaghetti code, I'm new to Javascript and want to get some insight into how seasoned Javascript developers would write code for such a task. My task was to build a tagging form which adds selected tags to hidden outputs to be sent to a server. 

var tags = [];
var createdTags;
var addedTags = [];
var isNewTag, selectedTag, i;

$("#created-tags").attr("value", "[]");

$.ajax({

    type: "GET",
    cache: false,
    dataType: 'json',
    url: "/portal/index.php/movies/get_tags",
    success:
        function(response) {
            $("#tags-typealong").autocomplete({
                source: response,
                minLength: 1,
                select: function (event, ui) {
                    event.preventDefault();             
                    selectedTag = ui.item;
                    //tags = current tags
                    tags = $.parseJSON($('#tags').attr('value'));
                    isNewTag = true;
                    //is tag already added?
                    for(i = 0; i < tags.length; i++) {
                        if(selectedTag.id == tags[i].id)
                            isNewTag = false;
                    }
                    //update addedTags if new
                    if(isNewTag) {
                        tags.push(selectedTag);
                        addedTags.push(selectedTag.id);
                        add_tag_render(selectedTag.name);
                    }
                    //update dom for tag_form.js
                    $("#tags").attr("value", JSON.stringify(tags));
                    $("#added-tags").attr("value", JSON.stringify(addedTags));
                    $("#tags-typealong").val('');
                }

            }).data( "autocomplete" )._renderItem = function( ul, item ) {
                return $("<li></li>")
                    .data( "item.autocomplete", item )
                    .append( '<a>' + item.name  + '</a>' )
                    .appendTo(ul);
            }
        }
});

$("#tags-typealong").keyup(function (event) {

    var isNew = true;

    if(event.keyCode == 32) {
        //get updated tag selections
        var tagName = $(this).val();
        tagName = tagName.slice(0, -1);
        createdTags = $.parseJSON($('#created-tags').attr('value'));
        tags = $.parseJSON($('#tags').attr('value'));
        //check tags from db
        for(i = 0; i < tags.length; i++) {
            if(tags[i].name == tagName)
                isNew = false;
        }
        //check existing created tags
        for(i = 0; i < createdTags.length; i++) {
            if(createdTags[i] == tagName)
                isNew = false;
        }
        if(isNew) {
            add_tag_render(tagName);
            createdTags.push(tagName);
            $("#created-tags").attr("value", JSON.stringify(createdTags));
        }
        $("#tags-typealong").val('');
        //TODO!
        //is tagname in db already?

    }

});

function add_tag_render(tagName) {

    $(".tag-container").append('<div class="selected-tag"><div class="selected-tag-name">' + tagName + '</div><div class="remove-tag"></div></div>');
    $("#tags-typealong").appendTo(".tag-container");
    $("#tags-typealong").focus();

}
share|improve this question

2 Answers

up vote 1 down vote
  • .attr('value') is bad. Always. You should use .val() instead.
  • var i; can go into the callback function instead of whatever scope you are in at the very beginning.
share|improve this answer
It's not leaking a global i since its declared at the top with the other var statements. – James Allardice Jun 17 '12 at 9:15
Ah, indeed. Well, it should be inside the callback function - no need to put it in a broader scope than necessary. Especially since the outermost scope in his code might very well be the global scope. – ThiefMaster Jun 17 '12 at 9:17
That's true, although that i is being used in both the AJAX callback and the keyup event callback. A new i could be declared in each scope, but I think for this code it's not too bad. – James Allardice Jun 17 '12 at 9:28
up vote 1 down vote

On the whole I don't think there's anything I would have done very differently. However, there are numerous ways your existing code can be improved (but not restructured, so I'm only going to talk about your code in its current form).

So let's start right at the beginning:

var tags = [];
var createdTags;
var addedTags = [];
var isNewTag, selectedTag, i;

The first thing that leaps out at me is the mixed style. Not a problem, but it would probably be sensible to stick to one. I (and JSLint) prefer the style of "one var statement per scope". The next thing I notice is that tags is assigned a value later on, so there is no point assigning it a value here. To sum up that paragraph in code:

var tags,
    createdTags,
    addedTags = [],
    isNewTag, 
    selectedTag,
    i;

On the next line you set the value of what is presumably an input element. The val method was designed specifically for that, so should probably be used here (and in the numerous other places you use attr):

$("#created-tags").val("[]");

The next line is your call to ajax. This can be shortened considerably because there is a jQuery method, getJSON, that behaves exactly the same as your ajax call (update - as pointed out in the comments, this isn't quite equivalent. It doesn't set the cache option. If it's important the the cache option is set then you'll have to stick with $.ajax):

$.getJSON("/portal/index.php/movies/get_tags", function(response) {
    //Your success event handler
});

Let's look at the body of your keyup event callback. The first thing that I notice here is the fact that you are searching the DOM for some elements every time it's called. Since keyup is probably triggered quite often, it would be more efficient to cache the results of your selectors. I would add the following declarations to the var statement at the top:

var tagsElem = $("#tags"),
    createdTagsElem = $("#created-tags"),
    tagsTypealong = $("#tags-typealong");

You can then use those identifiers in the keyup event callback (and the AJAX success callback) instead of having to search the DOM for those elements every time.

In the keyup callback you use both the jQuery $.parseJSON method and the native JSON.stringify method. Since you're using a native JSON method for stringifying, I would suggest using the equivalent native method for parsing too, JSON.parse, since it's going to be faster then that jQuery version. And don't forget to polyfill the native JSON methods so you can support older browsers.

share|improve this answer
$.getJSON does not use cache:false. Besides that, I think using $.ajax() is more readable since every argument is on the same indentation level. – ThiefMaster Jun 17 '12 at 9:36
Ahh, good point. I tend to prefer the shorthand versions, but you are right - if the cache option needs to be set in this case then there's no choice but $.ajax. – James Allardice Jun 17 '12 at 9:39
Or $.ajaxSetup({cache:false});.. that might actually be a good idea since usually you don't want any AJAX caching. – ThiefMaster Jun 17 '12 at 9:48

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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