Object serializer

Question

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

Answer 1

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;
}