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 have 5 different forms where I am disabling the submit button when a valid form is submitted so the form can only be submitted once. I am using a hidden field on each form to get the form Id.

function OnSubmit(buttonId) {
var disableButton = false;

if ($('#formType').val() == 1) {
    if ($('#CreateEntityName').val() != "" && $('#newAccountGroup').val() != "") {
        disableButton = true;
    }
}
if ($('#formType').val() == 2) {
    if ($('#newAccount').val() != "" && $('#lastName').val() != "") {
        disableButton = true;
    }
}
if ($('#formType').val() == 3) {
    if ($('#select2-entityId').val() != 0 && $('#contact').val() != "") {
        disableButton = true;
    }
}
if ($('#formType').val() == 4) {
    if ($('#select2-entityId').val() != 0 && $('#contact').val() != "" && $('#need').val() != "" && $('#candidates').val() != 0) {
        disableButton = true;
    }
}
if ($('#formType').val() == 5) {
    if ($('#newAccount').val() != "" && $('#country').val() != "") {
        disableButton = true;
    }
}
if (disableButton == true) {
    $('#' + buttonId + '').attr("disabled", true);
}
}

@Html.Hidden("formType", 5)

share|improve this question

1 Answer 1

up vote 1 down vote accepted

I see a few things that can be done to tweak this:

  • Cache your selectors instead of re-querying the DOM each time. Anything you use more than once should be cached.
  • Since all of the form fields use ID's you can use the native getElementById for faster performance.
  • Use strict equality when your are evaluating to reduce the chance of a gotcha.
  • Use 'use strict;' if you aren't already.
  • Surround your code by an IIFE, to encapsulate it and create a private scope.
  • Namespace your code

`

(function($){
  'use strict';
  var validateForm = {};

  //shortcut functions to simplify code below
  function isEmpty(str) { return str === ''; }
  function isNotSelected(num) { return num === 0; }
  function id(str) { return document.getElementById(str).value+''; }
  function getNum(str) { return document.getElementById(str).value-0; }

  function onSubmit( buttonId ) {
    var disableButton = false;
    var formType = getNum('formType');

    //cache selectors below
    var contact= id('contact');
    var newAccount =  id('newAccount');
    //assuming this is a dropdown
    var el = document.getElementById('select2-entityId');
    var entityId = el.options[ el.selectedIndex ].value -0;

    switch( formType ) {
       case 1:
        if ( isEmpty(id('CreateEntityName')) && isEmpty(id('newAccountGroup')) ) {
          disableButton = true;
        }
        break;

       case 2:
        if ( isEmpty(newAccount) && isEmpty(id('lastName')) ) {
          disableButton = true;
        }
        break;

       case 3:
        if ( isNotSelected(entityId) &&  isEmpty(contact) ) {
          disableButton = true;
        }
        break;

       case 4:
        if ( isNotSelected(entityId) && isEmpty(contact) && isEmpty(id('need')) && isNotSelected(getNum('candidates')) ) {
          disableButton = true;
        }
        break;

       case 5:
        if ( isEmpty(newAccount) && isEmpty(id('country')) ) {
          disableButton = true;
        }
        break;
    }

    if ( disableButton ) {
      $('#' + buttonId + '').prop('disabled', true);
    }

    return disableButton;
  }

  validateForm.init = onSubmit;
  window.validateForm = validateForm;

  //document.ready if you need to do something else
  $(function(){
    if ( validateForm.init() ) {
      //do something here
    }  
  });

})( jQuery );

I am sure there are other things that can be done. Hope that helps!

share|improve this answer

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.