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.

I have two scripts written by my co-workers for auto-filling a form with information from the facebook graph-api. I am trying to figure out which has been coded in a more efficient style?

This is the first one. http://jsfiddle.net/TcGGZ/30/

$('#account_get').click(function(){
    $.getJSON('http://graph.facebook.com/'+$('#facebook').val(), function(fd) {
        $('#id').val(fd.id);
        $('#name').val(fd.name);
        $('#first_name').val(fd.first_name);
        $('#last_name').val(fd.last_name);
        $('#link').val(fd.link);
        $('#username').val(fd.username);
        $('#gender').val(fd.gender);
        $('#locale').val(fd.locale);
    });
});​

This is the second one I am trying to decide about. http://jsfiddle.net/VuY2b/46/

var facebook_account = '';

function fillForm() {
    facebook_account = $('#facebook').val();

    $.getJSON('http://graph.facebook.com/' + facebook_account, function(data) {
        var facebook_data = data;
        console.log(facebook_data);
        var inputs = $('form > input');
        for (var key in facebook_data) {
            for (var l = 0; l < inputs.length; l++) {
                if (inputs[l].getAttribute('name') === key) {
                    inputs[l].value = facebook_data[key];
                }
            }
        }
    });

}

var x = document.getElementById('account_get');
x.addEventListener("click", fillForm, false);​
share|improve this question
    
On the second one, I would consider to do it reversed: for each input, if facebook_data[input.getAttribute('name')] has anything, then fill in the input. –  ANeves Nov 19 '12 at 14:36
add comment

2 Answers 2

The first one will be slow because of all the jQuery selection.

The second one will be slow because of the two loops. It keeps looping over what was already found. BUT it is going to be faster than the first one.

I made a basic jSPerf with a basic object: http://jsperf.com/some-random-beef

I also added two more cases to show you faster ways. NOW, if the object contains more junk in it, the results may differ.

var inputs = document.forms[0];
for (var key in fd) {
    var elem = inputs[key];
    if (elem) {
        elem.value = fd[key];
    }       
}

Now what version of the code would I want to read? The first one.

share|improve this answer
    
Isn't getting an element by ID really fast on any browser from the past 5 years? –  Peter Taylor Nov 15 '12 at 8:15
1  
this is just a snippet of what going in to the project, you should really consider the fact that the final form is gonna be pretty huge, we are collecting lots of data from multiple social media sites. The first choice might look better, and using document.getElementById might be faster, but in the end to write out a line of code for every single form element that needs to be filled would be less pleasant to look at then a loop that matches attribute names and fills in data when a match is found –  EnigmaMaster Nov 15 '12 at 15:31
add comment

The first one is very readable but is hard coded to specific ids and fields. This increases support costs. The ids of the html elements have a potential name conflict with any other forms you may add.

The second is harder to read but will adapt to changes in the model without changing JS. It also avoids the chance of id conflicts. This code also uses dom functions, so it doesn't give you the compatibility advantage afforded by jQuery.

you should probably abstract your automapper to its own utility class. That way you end up with the following:

var getFacebookData = function() {
    var path = 'http://graph.facebook.com/'+$('#facebook').val();
    var inputElements = $('form > input');
    $.getJSON(path, function(facebookData) { Utility.dataMapper( facebookData, inputElements )});
 };

$('#account_get').click( getFacebookData );

And add this to your utility class:

//General purpose method for copying a hash to corresponding input elements.
Utility.prototype.dataMapper = function( hash, elementList )
{
 for (var key in hash) {
        for (var l = 0; l < elementList.length; l++) {
            if (elementList[l].getAttribute('name') === key) {
                elementList[l].value = hash[key];
            }
        }
    }

}

I do alot of datamapping so something like dataMapper() can be re-used frequently. For UI operations you usually don't need to worry about performance as the time between user clicks or ajax requests is large compared to the time to copy over fields. the focus should be on readability and code support.

It is wise to use the name attribute to denote the field as you did with the name attribute. You can also use class attrivute for nicer looking jQuery searches, but it is much slower.

If you want to get the performance gains of epascarello's method, with the added performance gain of having the data mapper predefined in a utility library and have good looking concise code you can do this.

//Put dataMapper into your utility library.
Utility = {};
Utility.dataMapper = function(hash, elementList) {
    for (var key in hash){
        var element = elementList[key]; 
        if ( element ) element.value = hash[key];
    }
}
//use this in your initialization code
var getFacebookData = function() {
    var path = 'http://graph.facebook.com/'+$('#facebook').val();
    var inputElements = document.forms[0];
    $.getJSON(path, function(facebookData) { Utility.dataMapper( facebookData, inputElements )});
 };

$('#account_get').click( getFacebookData );

I benchmarked this on epascarello JSPerf - http://jsperf.com/some-random-beef/8 and it increased performance a slight amount from the grim reaper method I mostly copied. I think a combination of his performance increase and the concise code of this example would be your best bet.

share|improve this answer
add 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.