2
\$\begingroup\$

I created two dropdowns, a dadand a son that are populated based on two arrays that, hypothetically, can't be changed.

When you choose a dad with the code x it generates the son's drop-down with the options containing the code x.

var dadArray = [
  [1, 'Dad1'],
  [2, 'Dad2'],
  [3, 'Dad3']
];
var sonArray = [
  [1, 'Son1'],
  [1, 'Son2'],
  [3, 'Son3'],
  [2, 'Son4']
];


function sortArray(dad, dadID) {
  for (var i = 0; i < dad.length; i++) {
    $(dadID).html($(dadID).html() + '<option value="' + dad[i][0] + '">' + dad[i][1] + '</option>');
  }
  var selDad = $('#dadDrop option:selected').val();
  for (i = 0; i < sonArray.length; i++) {
    if (sonArray[i][0] == selDad) {
      $('#sonDrop').html($('#sonDrop').html() + '<option value="' + sonArray[i][0] + '">' + sonArray[i][1] + '</option>');
    }
  }
}

sortArray(dadArray, '#dadDrop');

$("#dadDrop").change(function() {
  $('#sonDrop').html('');
  var selDad = $('#dadDrop option:selected').val();
  for (var i = 0; i < sonArray.length; i++) {
    if (sonArray[i][0] == selDad) {
      $('#sonDrop').html($('#sonDrop').html() + '<option value="' + sonArray[i][0] + '">' + sonArray[i][1] + '</option>');
    }
  }
});
<script src="https://code.jquery.com/jquery-2.1.4.min.js"></script>
<select id="dadDrop">
</select>
<br />
<br />
<select id="sonDrop">
</select>
<br />
<br />

I made it using jQuery, but it looks huge (Probably because I'm still a newbie to JS and its libraries with amazing features). Is there a different logic that requires fewer lines? of course there is, but may you share it with me?

\$\endgroup\$
0

2 Answers 2

2
\$\begingroup\$

Your data doesn't mean much. I would suggest an array of objects so that the data will have meaning. Also, I would not recommend naming your variables based on data type. JS doesn't have strict types. You can easily assign anything to the variable. This means your dadArray may not be an array at some point.

var dads = [
  {id: 1, name: 'Dad1'},
  {id: 2, name: 'Dad2'},
];

var sons = [
  {id: 1, dadId: 1, name: 'Son1'},
  {id: 2, dadId: 1, name: 'Son2'},
  {id: 3, dadId: 3, name: 'Son3'},
  {id: 4, dadId: 2, name: 'Son4'},
];

So your operation involves:

  • Populating the dads dropdown.
  • On dad selection, populate the sons dropdown depending on the dad.

I notice that in your code there's duplicates. Let's split them up into functions to be reusable. Based on the operations, you should have something like:

function populateDads(dads){...}
function populateSons(sons){...}

Let's start first with the dads. I would discourage using .html(). While it does work, what happens is that jQuery does not clean up after itself. Handlers that are assigned to elements that are replaced will linger on your page. Suggesting you use .empty() if your current set is the parent of the elements to remove or .remove() if your current set are the elements to remove.

$('select').empty(); // Empties the options

As far as I know, jQuery's .val() should be able to extract the value from a <select>. No need to find the selected <option> from the <select>.

var selDad = $('#dadDrop').val();

If both your select boxes are static throughout the life of the page, you can simply just store their jQuery object in a variable. That way, you won't be querying the DOM every now and then.

var dadSelect = $('#dadDrop');
var sonSelect = $('#sonDrop');

There's also a neater way to construct elements in jQuery. It goes like:

$('<tagName />', { attr: value });

Running with all these, the code should look like:

var dads = [
  {id: 1, name: 'Dad1'},
  {id: 2, name: 'Dad2'},
  {id: 3, name: 'Dad3'},
];

var sons = [
  {id: 1, dadId: 1, name: 'Son1'},
  {id: 2, dadId: 1, name: 'Son2'},
  {id: 3, dadId: 3, name: 'Son3'},
  {id: 4, dadId: 2, name: 'Son4'},
];

var dadSelect = $('#dadDrop');
var sonSelect = $('#sonDrop');

function populateDads(dads){
  dadSelect.empty();
  $.each(dads, function(index, dad){
    $('<option />', {value: dad.id}).text(dad.name).appendTo(dadSelect);
  });
}

function populateSons(sons){
  sonSelect.empty();
  $.each(sons, function(index, son){
    $('<option />', {value: son.id}).text(son.name).appendTo(sonSelect);
  });
}

dadSelect.on('change', function(){
  // .val() returns a string. Coerce the value to a number using the
  // prefix "+". Acts like parseFloat most of the time.
  var selectedDad = +dadSelect.val();
  var dadSons = sons.filter(function(son){ return son.dadId === selectedDad; });
  populateSons(dadSons);
});


populateDads(dads);

// Trigger change for first load to prepopulate children
dadSelect.trigger('change');
<script src="https://ajax.googleapis.com/ajax/libs/jquery/1.11.1/jquery.min.js"></script>
<select id="dadDrop"></select>
<select id="sonDrop"></select>

\$\endgroup\$
1
  • \$\begingroup\$ dadSelect.trigger('change'); saved a bunch of used-only-once code, Thank you so much +1 \$\endgroup\$
    – Kyle
    Commented Oct 1, 2015 at 13:27
1
\$\begingroup\$

Here are a few suggestions.

Use more JQuery to make populating initial dropdown neater and easier to read

For starters, since you are already using JQuery, I'd recommend using more of it for your code. For example, to populate a dropdown with a given array use something like the following (lifted from this SO post):

$.each(selectValues, function(key, value) {   
     $('#mySelect')
         .append($("<option></option>")
         .attr("value",key)
         .text(value)); 
});

You can use this for the initial populating action and also (with a bit of modification to add an if statement) in the second, more selective populating action.

Change the structure of your data if you can

Does your data have to be stored in its current form? It might make more sense to store an array of sons for each dad index. So your son object could look more like

var sons = { 1: ['Son1', 'Son2'],
             2: ['Son4'],
             3: ['Son3']
           }

This will spare you having to check all sons when actually your data structure already should know which ones you want. Then say dad1 is selected, your selectValues for the code above becomes sons[1].

Change your variable names

dad and son variants are quite specific and will not make this code portable. Also sortArray is a confusing name for your first function considering that it doesn't do any sorting. I take it this is in the British sense of 'take care' rather than sort, but it might be better to name it something like initRelatedDropDowns.

Keep your functions single purpose

Your function sortArray is doing a lot. Should it really be taking care of the second array as well? Seems like the second function is doing the same work, so it's a bit confusing to have this repetitive code. You should probably name that second function and call it for the initial load as well as on change.

\$\endgroup\$

Your Answer

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

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