I'd change two things. I'd start by declaring your var's in one statement like so:

function test_submit(id){    var q=[],        p=$(id).getElementsByTagName('SELECT');

Second I'd use more descriptive variable names than i, p, r, n. If you run this code through a minifier it'll do this for you. When you come back to edit this code in a few months you'll want to know exactly what each variable is for, and using a descriptive name helps with that.

I'd also be leary of using $ in the global namespace as a variable name since a lot of the popular libraries use it and you don't want to have any collisions with those.

• \$\begingroup\$Understood! Made the appropriate changes. You are right, I often minify the code by hand when it is not necessary.\$\endgroup\$

  steveo225

  – steveo225

  2011-09-30 21:03:37 +00:00

  CommentedSep 30, 2011 at 21:03
• \$\begingroup\$@steveo225 So my answer was good, just not "upvote" good? :-)\$\endgroup\$

  bittersweetryan

  – bittersweetryan

  2011-09-30 21:32:54 +00:00

  CommentedSep 30, 2011 at 21:32
• \$\begingroup\$Just taking my time...\$\endgroup\$

  steveo225

  – steveo225

  2011-09-30 21:53:55 +00:00

  CommentedSep 30, 2011 at 21:53

In regards to variablesi andj they are loop index variables and can be declared in the loop alone instead of at the top. It will clean up the code.

Example

for (var j=0;j<obj.length;++j){...}

Personally I would write it up like this describingp andi which are form elements. In additon there is no need to duplicate code (example would be 3 identical for statments), you can gather all the form elements and concat the arrays together in one. Then by using a switch statement you can minimize the duplicateifs for each element type and eliminate the need for duplicate for statements. Have a look below. it is much cleaner.

function test_submit(id){      var params=[],        results=[],        form=document.getElementById(id),        obj;      for(var i = 0, var formElement = form.elements[i]; i < form.elements.length; i++) {          if (formElement.name) {            switch (formElement.tagName) {                case "SELECT":                    for(var j=0, var optionItem = formElement.options[j]; j < formElement.length; j++){                          if(optionItem.selected){                              var value = optionItem.value;                              if(!value) value = optionItem.text;                              params.push([formElement.name,value]);                          }                      }                      break;                case "INPUT":                    var type=formElement.type,                        value=formElement.value;                      if(type) type=type.toLowerCase();                      if(type && (type == 'radio' || type == 'checkbox')) {                          if(formElement.checked) params.push([formElement.name,value?value:'on']);                      }                      else params.push([formElement.name,value]);                      break;                case "TEXTAREA":                    params.push([formElement.name,formElement.value]);                      break;            }        }    }    for (var i in params) results.push(params[i][0]+'='+escape(params[i][1]));      return results.join('&');  }

• \$\begingroup\$The only issue I see,getElementsByTagName returns an object, not array, soconcat() doesn't work\$\endgroup\$

  steveo225

  – steveo225

  2011-10-01 00:48:17 +00:00

  CommentedOct 1, 2011 at 0:48
• \$\begingroup\$@stevo225 actually what was I thinking you can simply iterate over form.elements. The above code corrects my brain fart of trying to concat multiple HTMLCollections. Sorry about that.\$\endgroup\$

  John Hartsock

  – John Hartsock

  2011-10-01 01:35:41 +00:00

  CommentedOct 1, 2011 at 1:35
• \$\begingroup\$I like that, no usinggetElementsByTagName and they come in the proper order (not that it matters). I made a couple edits for my own taste.\$\endgroup\$

  steveo225

  – steveo225

  2011-10-01 12:26:09 +00:00

  CommentedOct 1, 2011 at 12:26

Elegant solution overall. Try jQuery as it has some nice features that can let you do some of this in one-liners. Code-on brother.

• \$\begingroup\$I know, but I was looking for a non-jQuery solution.\$\endgroup\$

  steveo225

  – steveo225

  2011-10-09 13:00:56 +00:00

  CommentedOct 9, 2011 at 13:00

A few things:

1 I'd like to see it handle password fields separately: you might want to process them in some way, obfuscation, validation, strength testing etc.

2 Personally, I'd avoid using 'form' as a variable name. Just for the sake of my own sanity, if nothing else.

3 What if the form elements have nonames?

• \$\begingroup\$1) The best approach would be to add a callback that handlespassword fields, otherwise it isn't standard and wouldn't behave like the browser, also password fields as not always used for passwords, so strength testing and validation don't always apply\$\endgroup\$

  steveo225

  – steveo225

  2011-10-04 11:51:31 +00:00

  CommentedOct 4, 2011 at 11:51
• \$\begingroup\$2) I usedform because that is what it points to, the form, also it is just a local variable, but that is really user preference\$\endgroup\$

  steveo225

  – steveo225

  2011-10-04 11:52:27 +00:00

  CommentedOct 4, 2011 at 11:52
• \$\begingroup\$3) That is handled. Look at the firstif statement in thefor loop:if(obj.name) ...\$\endgroup\$

  steveo225

  – steveo225

  2011-10-04 11:53:43 +00:00

  CommentedOct 4, 2011 at 11:53
• \$\begingroup\$3 Ah... I didn't make myself clear. I meant: what if the form inputs are identified via ID attributes, not NAMEs?\$\endgroup\$

  graphicdivine

  – graphicdivine

  2011-10-04 12:27:37 +00:00

  CommentedOct 4, 2011 at 12:27
• \$\begingroup\$To be sent by the browser, the name attribute is required, that is how I modeled this function, to work exactly how the browser would work if the form were actually submitted.\$\endgroup\$

  steveo225

  – steveo225

  2011-10-04 13:48:43 +00:00

  CommentedOct 4, 2011 at 13:48

• javascript