current community

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 3 down vote favorite

There's things I know about Javascript and jQuery, and things I don't. I know that this works, gets and stores what I want so that, when I need to do stuff, I don't have to wait for it. Functionally, I like this. But I'm the only Javascript guy in my shop, and so I don't see Javascript I didn't write all that often.

I've heard recently that it is considered wrong to fill up the name space, so you use objects to hold everything. I think I'm doing this right now, but I'm not sure.

And in adding stuff to the DOM, I believe I'm a bit more verbose than I need to be, but I don't really know what to change.

Any comments?

// This uses Javascript, jQuery and jQuery-UI to allow for the addition of accession analysis stubs
// to new requests and accessions

var aa_stub = {

    // variables
    ajax : 'MY URL HERE' ,

    // data storage
    data : {} ,
    references      : {} ,
    reference_order : [] ,
    analysis        : {} ,
    analysis_order  : [] ,

    // functions

    // ---- ---- ---- ---- ---- ---- ---- ---- ---- ---- ---- ---- ---- ---- ---- ----
    // dialog popped up when "Add AA Stub' button is pressed
    do_dialog  : function ( sample_num ) {
        $( '<div/>' )
            .attr( 'id' , 'add_new_stub' )
            .attr( 'title' , 'Add Stub' )
            .dialog()
        $('.ui-icon-closethick').text( 'X' )
        $('.ui-dialog-titlebar-close')
            .click( function () {
                $( '#add_new_stub' ).remove()
                } )
        $( '<div/>' )
            .appendTo( '#add_new_stub' )
            .attr( 'id' , 'instructions' )
            .text( 'Enter a reference, an analysis, any extra parameters, then click Go' )

        $( '<fieldset/>' )
            .appendTo( '#add_new_stub' )
            .attr( 'id' , 'ref_box' )
        $( '<legend/>' )
            .appendTo( '#ref_box' )
            .text( 'Reference Species' )
        $( '<select/>' )
            .appendTo( '#ref_box' )
            .attr('id' ,'reference')
        for ( i in aa_stub.reference_order ) {
            var j = aa_stub.reference_order[i]
            var ref = aa_stub.references[j]
            $( '<option/>' )
                .text( ref )
                .val( j )
                .appendTo( '#reference' )
            }

        $( '<fieldset/>' )
            .appendTo( '#add_new_stub' )
            .attr( 'id' , 'anal_box' )
        $( '<legend/>' )
            .appendTo( '#anal_box' )
            .text( 'Analysis Type' )
        $( '<select/>' )
            .appendTo( '#anal_box' )
            .attr('id' ,'analysis')
        for ( i in aa_stub.analysis_order ) {
            var j = aa_stub.analysis_order[i]
            var analysis = aa_stub.analysis[j]
            $( '<option/>' )
                .text( analysis )
                .val( j )
                .appendTo( '#analysis' )
            }

        $( '<fieldset/>' )
            .appendTo( '#add_new_stub' )
            .attr( 'id' , 'param_box' )
        $( '<legend/>' )
            .appendTo( '#param_box' )
            .text( 'Extra Parameters' )
        $( '<input type="text"/>' )
            .appendTo( '#param_box' )
            .attr('id' ,'extra_parameters')
        $( '<input type="hidden"/>' )
            .appendTo( '#param_box' )
            .attr('id' ,'sample')
            .val( sample_num )

        $( '<div/>' )
            .attr( 'id' , 'stub_go' )
            .text( 'Go' )
            .appendTo( '#add_new_stub' )
            .click( function () {
                var reference  = $( '#reference' ).val()
                var analysis   = $( '#analysis' ).val()
                var parameters = $( '#extra_parameters' ).val()
                var sample     = $( '#sample' ).val()
                $('.ui-icon-closethick').click( )
                if ( sample === '' ) {
                    $( '.stub_bucket' ).each( function () {
                        var sample = $( this ).attr( 'sample' )
                        aa_stub.add_to_bucket( sample , reference , analysis , parameters )
                        } )
                    }
                else {
                    aa_stub.add_to_bucket( sample , reference , analysis , parameters )
                    }
                } )
        } ,

    // ---- ---- ---- ---- ---- ---- ---- ---- ---- ---- ---- ---- ---- ---- ---- ----
    // adding AA stubs to the form
    add_to_bucket : function ( sample , reference , analysis , parameters ) {
        var bucket = '#stub_bucket_' + sample
        var ref_text = aa_stub.references[ reference ]
        var a_text = aa_stub.analysis[ analysis ]

        var number = 1
        $( bucket ).children( 'span' ).each( function () {
            var num = parseInt( $( this ).attr( 'number' ) )
            if ( number <= num ) { number = 1 + num }
            } )
        var id =  [ 'span' , sample , number ].join('_')
        $( '<span/>' )
            .addClass( 'stub' )
            .attr( 'number' , number )
            .attr( 'id' , id )
            .appendTo( bucket )
            .text( [ ref_text , a_text ].join( ' - ' ) )
            .attr( 'title' , 'Click to delete' )
            .click( function () {
                $(this).remove()
                } )
        $( '<input type="hidden">' )
            .attr( 'name' ,  [ 'reference' , sample , number ].join('_') )
            .val( reference )
            .appendTo( '#' + id )
        $( '<input type="hidden">' )
            .attr( 'name' ,  [ 'analysis' , sample , number ].join('_') )
            .val( analysis )
            .appendTo( '#' + id )
        $( '<input type="hidden">' )
            .attr( 'name' ,  [ 'parameters' , sample , number ].join('_') )
            .val( parameters )
            .appendTo( '#' + id )
        } ,

    // ---- ---- ---- ---- ---- ---- ---- ---- ---- ---- ---- ---- ---- ---- ---- ----
    // the startup part
    initialize : function ( ) {
        aa_stub.data.flag = 1
        // fill data
        var url = aa_stub.ajax
        $.post( url , function ( data ) {
            for ( i in data.references ) {
                var ref = data.references[ i ]
                aa_stub.references[ i ] = ref
                }
            for ( i in data.analysis ) {
                var ref = data.analysis[ i ]
                aa_stub.analysis[ i ] = ref
                }

            for ( i in data.references_order ) {
                var j = data.references_order[ i ]
                aa_stub.reference_order.push( j )
                }
            for ( i in data.analysis_order ) {
                var j = data.analysis_order[ i ]
                aa_stub.analysis_order.push( j )
                }
            } , 'json' )
        // get all
        $( '#add_stubs_to_all' )
            .click( function () {
                aa_stub.do_dialog( '' )
                } )
        // get one
        $( '.stub_add' )
            .click( function () {
                var sample = $( this ).attr('sample')
                aa_stub.do_dialog( sample )
                } )
        }

    }

$( function() {
    aa_stub.initialize()
    } )
share|improve this question
add comment

1 Answer

up vote 6 down vote

Let me start with few common observations.

It's better to generate a complete DOM element before you attach it to DOM tree (add to page) if you can, because it tends to be faster. For example setting its text after can trigger another reflow. Do it often enough and it start to matter. Not every change is expensive (e.g. setting ID before or later doesn't matter much), but I think it also reads better (step 1.: build node, step 2: attach it).

If you are building a more complex HTML (DOM subtree), it's also better to create a document fragment, add nodes to it and then add that fragment to page to avoid costly reflows.

It's good practice to declare all your variables at the top of function. They will exist from that point onward even if you declare them at the very bottom. Not doing this opens you to subtle errors.

Not every variable has to have its own var statement infront. E.g. it's OK to do var a = 5, b = 3 to declare a and b;

So on to concrete examples:

$( '<div/>' )
    .attr( 'id' , 'add_new_stub' )
    .attr( 'title' , 'Add Stub' )
    .dialog()

You could do this same thing in two ways:

$('<div id="add_new_stup" title="Add Stub"></div>').dialog();

or

$('<div />', { id: 'add_new_stub', title: 'Add Stub })
    .dialog()

Personally I'd pick first version when dealing with elements that have fixed attribute values, since it's most obvious and second when attribute values are variables. Learn more.

Instead of for() loop over array, you can use

$.each(array, function (i, el) {
    ... your code ...
});

This way you can also localize variables better.

I don't understand the purpose of this part:

    var number = 1
    $( bucket ).children( 'span' ).each( function () {
        var num = parseInt( $( this ).attr( 'number' ) )
        if ( number <= num ) { number = 1 + num }
        } )
    var id =  [ 'span' , sample , number ].join('_')
    $( '<span/>' )
        .addClass( 'stub' )
        .attr( 'number' , number )

Correct me if I'm wrong, but you are trying to set number to 1 higher than highest, right? If yes, than that's an awfully expensive way to do this. You could improve this in two ways. Either you store current maximum or since new span with highest number is always added to the end of a list, just pick that one ("span:last") and read what maximum currently is.

You don't use aa_stub.data.flag anywhere.

Btw, I'm not fond of dropped colons at the end of lines and closing } at same depth as end of block, but that's a personal choice.

Anyway, that's a quick overlook of your code. I hope it helped.

share|improve this answer
 
It did, markos. I mostly use the $( "<div id='foo'/>" ) style with inputs, because if jQuery will create or modify the TYPE attribute, I don't know it. I did not know about document fragments. Will try that. It might work similarly if I create an element, add to it, and append it to the page as the last thing. Which gets to a thing I'm curious about: I think I've seen code where you define, for example, an LI inline with the UL it exists in, but I've forgotten where I saw it. aa_stub.data.flag is orphaned and forgotten. –  VarLogRant Aug 29 '11 at 13:41
 
I got it from somewhere, Crockford maybe?, that ending semicolons are superfluous and should be avoided. I've been trying to get them out of my Javascript, in part so that I don't mentally confuse it with my Perl and end up getting errors by conflating "my" with "var" and vice versa. –  VarLogRant Aug 29 '11 at 19:45
 
I guess I knew about .last() but forgot about it. Changed that. –  VarLogRant Aug 31 '11 at 18:21
2  
Semicolons mostly are not needed and some argue they should be dropped, but Crockford isn't one of those people :) I personally use them, but as a Python developer am not really attached to them. –  markos Sep 5 '11 at 15:35
 
I'm not exactly sure what you meant with inline LI comment, but you can add one either by creating whole HTML if necessary (like $("<ul><li>item</li></ul") or if it already exist, by using a method like append ($("ul").append("<li>item</li>"). –  markos Sep 5 '11 at 15:37
show 1 more comment

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.