4
\$\begingroup\$

I have the following function, it works exactly how I need it to; however I think there is room for improvement.

The script is supposed to loop through all input and text area html elements. It should only work with elements that have a name, and a value, and it should ignore "hidden". However some of the elments do not have a type attribute, so I can't do something like input.attr('type').toLowerCase() != 'hidden'.

After that, it ignores any field with the class element_reference_input, and looks for fields with form-control or question_textarea_input.

Last it writes the values to an array.

jQuery('input, textarea').each(function(index){  
    var input = jQuery(this);
    if (input.val() && input.attr('name') && input.attr('type') != 'hidden' && input.attr('type') != 'HIDDEN') {
        if (input.attr('class')) {
            elmClass = input.attr('class');
        }

        if (elmClass.indexOf('element_reference_input') <= -1 && (elmClass.indexOf('form-control') > -1 || elmClass.indexOf('question_textarea_input') > -1 )) {
            objOutgoingData.push(
                {
                    name:'' + input.attr('name').split('.').pop(),
                    value:'' + input.val(),
                    table:'' + tableName,
                }
            );          
        }
    }
}); 

I feel that I have to many if statements, I'd like to streamline it. But I am trying to avoid undefined errors if class or type is not present on the html tag.

\$\endgroup\$
0

2 Answers 2

3
\$\begingroup\$

It should only work with elements that have a name, and a value, and it should ignore "hidden". However some of the elments do not have a type flag, so I can't do something like input.attr('type').toLowerCase() != 'hidden'.

After that, it ignores any field with the class element_reference_input, and looks for fields with form-control or question_textarea_input.

You can put all that in a single if-statement and it doesn't even have to look bad (you can probably format it a bit more to your liking):

if (input.val() && 
    input.attr('name') && 
    (
      !input.attr('type') ||    // no type defaults to `type="text"`
       input.attr('type').toLowerCase() != 'hidden'
    ) &&
    input.attr('class') &&
    input.attr('class').indexOf('element_reference_input') == -1 &&
    (  
      input.attr('class').indexOf('form-control') > -1 || 
      input.attr('class').indexOf('question_textarea_input') > -1 
    )
){
        objOutgoingData.push(
            {
                name:'' + input.attr('name').split('.').pop(),
                value:'' + input.val(),
                table:'' + tableName,
            }
        );
 }
\$\endgroup\$
3
  • \$\begingroup\$ @Shomz Thank you, this does look a lot better, and I understand the reasoning. I didn't consider doing the 'not' input type with the or condition. \$\endgroup\$
    – Nick
    Nov 11, 2015 at 15:27
  • \$\begingroup\$ You're welcome, there's not much more you could do there - maybe for the sake of readibility move all the checks into an external function and then simply do: if ( check(input) ) {... \$\endgroup\$
    – Shomz
    Nov 11, 2015 at 15:46
  • 1
    \$\begingroup\$ Thanks again! That's a good idea. I may consider moving it into it's own function, as I am debating adding more flexibility to filter for /or/ against additional classes dynamically. \$\endgroup\$
    – Nick
    Nov 11, 2015 at 16:15
4
\$\begingroup\$

You're failing to use the power of jQuery selectors, making the problem much harder than it needs to be.

Furthermore, elmClass = input.attr('class') is buggy. The elmClass variable is not localized. If the element (and all previously match elements!) has no class attribute, then the .indexOf() tests will crash.

Since input could be either an <input> or a <textarea>, I'd prefer a more generic variable name.

var tableName = 'some-table';

var objOutgoingData = jQuery(
       'input.form-control[name]:visible,' +
       'textarea.form-control[name],' +
       'textarea.question_textarea_input[name]')
  .not('.element_reference_input')
  .map(function() {
    var $element = jQuery(this);
    return {
        name:  $element.attr('name').split('.').pop(),
        value: $element.val(),
        table: tableName,
    };  
}).get();

jQuery('#output').focus(function() {
    $(this).val(
        JSON.stringify(objOutgoingData)
            .replace(/[{\]]/g, '\n$&')
    );
});
input, textarea { font-family: monospace; background-color: #f99; }
.incl { background-color: #9f9; }
#output { background-color: inherit; }
<script src="https://ajax.googleapis.com/ajax/libs/jquery/1.11.1/jquery.min.js"></script>
<form>
  <textarea id="output" rows="6" cols="80">Click me to generate objOutgoingData.
Green elements are to be included; red ones and invisible ones excluded.
</textarea>

  <input type="text"   name="input.noclass"                           value="untyped input (no class)">
  <input               name="input.untyped" class="incl form-control" value="untyped input (form-control)">
  <input type="text"   name="input.text"    class="incl form-control" value="text input (form-control)">
  <input type="hidden" name="input.hidden"  class="excl form-control" value="hidden input (form-control)">

  <textarea               name="textarea"         class="incl question_textarea_input">textarea (question_textarea_input)</textarea>
  <textarea type="hidden" name="textarea.hidden"  class="excl">textarea (wrong class)</textarea>
</form>

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.