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 this little utility/helper which serializes a form into a javaScript object

            $.fn.serializeObject = function(fn) {
              var o = {},
                  a = this.serializeArray(),
                  that;
              $.each(a, function() {
                that = this;
                  if($.isFunction(fn)){
                    that = fn(that.name, that.value);
                  }
                if(o[this.name] !== undefined) {
                  val = that.value || '';
                  if(!o[that.name].push) {
                    o[that.name] = [o[that.name]];
                  }
                  o[that.name].push(that.value);
                } else {
                  o[that.name] = that.value;
                }
              });
              return o;
            };

A typical usage of this plugin will be:

            someNameSpace = {
                //... 
                processForm: function (form) {
                    return JSON.stringify(form.serializeObject(function (key, val) {
                        return {
                            name: key,
                            value: encodeURIComponent(val)
                        };
                    }));
                }
                //...
            };

Any suggestions on how to improve this piece of code?

  • Maybe improve performance
  • Maybe its the API-Design
  • Code readability..

    See original gist

  • share|improve this question

    1 Answer 1

    up vote 2 down vote accepted

    First of all, I'm barely even sure what's going on here:

    if(o[this.name] !== undefined) {
      val = that.value || '';
      if(!o[that.name].push) {
        o[that.name] = [o[that.name]];
      }
      o[that.name].push(that.value);
    } else {
      o[that.name] = that.value;
    }
    

    All the this/that is pretty confusing to look at, especially with all the square brackets mixed in. More descriptive names would help immensely.

    But also, you check for o[this.name] but then you define o[that.name]. So if my fn function makes all the names identical, it won't work. For instance, if I have 3 inputs (a = 1, b = 2, and c = 3) in a form, and I supply a fn like so:

    $(form).serializeObject(function (name, value) {
      return {
        name: "x",
        value: value
      };
    });
    

    I get:

    { "x": 3 }
    

    instead of the expected x: [1, 2, 3]

    It'd be much easier to just let me manipulate the objects directly. You wouldn't have any this/that to bother with, and I could simplify my fn to

    $(form).serializeObject(function (object) {
      object.name = "x";
    });
    

    No need to build and return a new object.

    Other things I noticed:

    • Don't use ... === undefined. Unfortunately, undefined is not a reserved word, meaning that undefined can be defined in some JS runtimes. So if somewhere, some joker has written undefined = 23; it'll break your code. Use typeof x === 'undefined' instead.

    • Don't use jQuery to do a simple loop. A for loop works quite well, with less overhead and no context-switching.

    • Don't use jQuery just to check it something's function. Use typeof x === 'function' instead.

    • Don't use x.push to check if something's an array. Use x instanceof Array. For all you know, x.push could mean anything. (User winner-joiner adds: watch out for frame/iframe situations where instanceof doesn't work)

    • You have a val variable which is 1) not used, 2) not declared (thus implicitly global!), and 3) if it were used, it'd mess up. I my fn returns a value of, say, false on purpose, it'd become an empty string, because false || '' => ''. Don't do anything clever with the values; especially not after the user's fn may have changed them. Assume the values are correct, regardless of what they are.

    And again: Use descriptive variable names. It took me quite a while to figure out your code.

    I'd write it like this:

    $.fn.serializeObject = function (decorator) {
      var source = this.serializeArray(),
          serialized = {},
          i, l, object;
    
      for( i = 0, l = source.length ; i < l ; i++ ) {
        object = source[i];
    
        if( typeof decorator === 'function' ) {
          decorator(object);
        }
    
        if( serialized.hasOwnProperty(object.name) ) {
          if( !(serialized[object.name] instanceof Array) ) {
            serialized[object.name] = [ serialized[object.name] ];
          }
          serialized[object.name].push(object.value);
        } else {
          serialized[object.name] = object.value;
        }
      }
      return serialized;
    }
    
    share|improve this answer
    1  
    +1 great and complete answer, i learned a lot Thx (would vote even more if possible) –  winner_joiner Mar 6 '13 at 7:41
        
    Thanks so much! really helpful! I will update the gist according to your recommendations and would you mind to please take a second look at it? –  adardesign Mar 6 '13 at 14:00

    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.