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 am trying to make a generic form parser for Javascript. The idea being, a developer could drop the function onto a form and sumbit via AJAX, or do something with a form besides submit it. The result will be in the same format used in a query string.

What I have posted works, so I am basically wondering if this is a good design? What could be improved? Thanks for the help!

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

4 Answers

up vote 3 down vote accepted

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.

share|improve this answer
    
Understood! Made the appropriate changes. You are right, I often minify the code by hand when it is not necessary. –  steveo225 Sep 30 '11 at 21:03
    
@steveo225 So my answer was good, just not "upvote" good? :-) –  bittersweetryan Sep 30 '11 at 21:32
    
Just taking my time... –  steveo225 Sep 30 '11 at 21:53
add comment

In regards to variables i and j 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 describing p and i 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 duplicate ifs 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('&');  
}  
share|improve this answer
    
The only issue I see, getElementsByTagName returns an object, not array, so concat() doesn't work –  steveo225 Oct 1 '11 at 0:48
    
@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. –  John Hartsock Oct 1 '11 at 1:35
    
I like that, no using getElementsByTagName and they come in the proper order (not that it matters). I made a couple edits for my own taste. –  steveo225 Oct 1 '11 at 12:26
add comment

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.

share|improve this answer
    
I know, but I was looking for a non-jQuery solution. –  steveo225 Oct 9 '11 at 13:00
add comment

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 no names?

share|improve this answer
    
1) The best approach would be to add a callback that handles password 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 –  steveo225 Oct 4 '11 at 11:51
    
2) I used form because that is what it points to, the form, also it is just a local variable, but that is really user preference –  steveo225 Oct 4 '11 at 11:52
    
3) That is handled. Look at the first if statement in the for loop: if(obj.name) ... –  steveo225 Oct 4 '11 at 11:53
    
3 Ah... I didn't make myself clear. I meant: what if the form inputs are identified via ID attributes, not NAMEs? –  graphicdivine Oct 4 '11 at 12:27
    
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. –  steveo225 Oct 4 '11 at 13:48
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.