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
    Commented 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
    Commented 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
    Commented 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\$

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.