Multiple drop down validation

Question

I have written some jQuery which validates a group of drop downs. It checks the text value selected in all the drop downs (first three characters), and if there is a duplicate, it warns the user.

This code seems quite messy, I was wondering if anyone would like to attempt to refactor this. Tips for improvement are welcome.

Here is the link to the Fiddle.

My main function:

          function validateTeam(section) {
          //array for storing selected nationalities:
          var $selectionArr = [];

          $('#' + section + '> .form-group select').each(function () {

              //strip the selected text:
              var $optText = $(this).find('option:selected').text().substr(0, 3);

              if ($optText.length == 3) {
                  $selectionArr.push($optText);
              }
          });

          //unique array of nationalities:
          var result = [];

          $.each($selectionArr, function (index, value) {
              if ($.inArray(value, result) == -1) {
                  result.push(value);

                  if (result.length == 5) {
                      $('#warning').removeClass('label-danger').addClass('label-success').text('Thank you - your selection is valid!');
                  } else {
                      $('#warning').text('');
                  }

              } else {
                  //console.log(value);
                  $('#warning').addClass('label-danger').text('Warning! You have selected more than one players from the same nation (' + value + ')');
              }
          });
      };

Answer 1

You didn't incude it in your post, but I'm comment on it anyway: The way you assign the validator function to your select elements isn't very optimal:

      $('#MainContent_ddOpenBrd1').change(function () {
          validateTeam('openSelect');
      });
      $('#MainContent_ddOpenBrd2').change(function () {
          validateTeam('openSelect');
      });
      $('#MainContent_ddOpenBrd3').change(function () {
          validateTeam('openSelect');
      });
      $('#MainContent_ddOpenBrd4').change(function () {
          validateTeam('openSelect');
      });
      $('#MainContent_ddOpenBrd5').change(function () {
          validateTeam('openSelect');
      });

You've got five calls all doing the same thing. You should at least combine them into one:

      $('#MainContent_ddOpenBrd1, #MainContent_ddOpenBrd2, #MainContent_ddOpenBrd3, #MainContent_ddOpenBrd4, #MainContent_ddOpenBrd5').change(function () {
          validateTeam('openSelect');
      });

Even better would be to select them with a more optimal selector such as a common class, or in this case a desendent selector:

      $('#openSelect select').change(function () {
          validateTeam('openSelect');
      });

This assigns the handler to each elements separately. You can go yet another step and use a delegated event handler, so that in the end you'll have a single handler assigned to a single element:

      $('#openSelect').on('change', 'select', function () {
          validateTeam('openSelect');
      });

And now you can get rid of the hard-coded ID as the parameter for your function:

      $('#openSelect').on('change', 'select', function (event) {
          validateTeam(event.delegateTarget.id);
      });

So, if you have multiple independent groups you can assign them all with one call - even if you don't reference them by their IDs, but for example a class:

      $('.select-group').on('change', 'select', function (event) {
          validateTeam(event.delegateTarget.id);
      });

---

Speaking of the function parameter: Instead of passing the ID you should directly pass a reference to that element - and with the change from above it's simple:

      $('#openSelect').on('change', 'select', function (event) {
          validateTeam(event.delegateTarget);
      });

So the start of your function changes to:

      $(section).find('> .form-group select').each(function () {

---

A short point about your variable names: You should only use $ as a prefix if the variable contains a reference to a jQuery object. Neither $selectionArr nor $optText do that. Also $selectionArr is a bad name. Something like selectedCountries would be better.

---

One impracticable thing you are doing, is reading/parsing the country abbreviation from the display label of the option. Generally trying to get information from a text that is intended for the user to read is a bad thing, since it can change at any time, e.g.

• the abbreviation is moved to the end of the string
• the full name of the country should be displayed
• the abbreviation/name is replaced with an icon
• etc.

Any of those events requires you to change your validation function.

Instead you should store such important information independently from the display string, for example in a data-* attribute:

<option value="11700599" data-country="AFG">AFG Farazi Khaiber (2134)</option>

and get it from there:

      var country = $(this).find('option:selected').data('country');

      if (country) { // No need to check the length, just if the value is set at all
        $selectionArr.push(country);
      }

---

You can also optimize the reading of the countries, for one by directly selecting the options in the "outer" selector (and also limiting it to ones that actually have a data-country attribute), and for another by usings jQuery's .map() function to convert the list of options directly into a list of strings:

         var $selectionArr = $(section).find('> .form-group option[data-country]:selected').map(function () {
           return $(this).data('country');
         });

---

That's all for now. If I have time I'll come back later.

EDIT: Some more:

Now to the validation loop:

You can optimize the checking of duplicates by using an object like a set instead of an array. That saves you having to search the array, but however requires you to count the number of entries yourself.

You should also also move the updating of the warning label to after the loop, since currently you are unnecessarily updating it in each run through the loop.

Finally you shouldn't hard code the ID for the warning label, because it won't work if you have multiple groups. Instead give it a class and search for it inside the group. Also it would be better to show/hide it instead of just clearing the text.

var countrySet = {};
var countryCount = 0;
var hasDuplicates = false;

$.each($selectionArr, function (index, country) {
    if (countrySet[country]) {
        hasDuplicates = true;
    } else {
        countrySet[country] = true;
        countryCount++;
    }
});

var warningLabel = $(section).find('.warning');

if (!hasDuplicates && countryCount === 5) {
   warningLabel.removeClass('label-danger').addClass('label-success').text('Valid!').show();
} else if (hasDuplicates) {
   warningLabel.removeClass('label-success').addClass('label-danger').text('Duplicates!').show();
} else {
    warningLabel.hide();
}

I'll leave displaying the duplicate country as an exercise :-)