Skip to main content
Code Review

Return to Answer

added 1167 characters in body
Source Link
Mike Brant
Mike Brant
  • 9.9k
  • 14
  • 24

UPDATE

To answer your question about how to get rid of your anti-pattern around id properties. Consider an <li> element that looks like this:

<li class="list-group-item justify-content-between" data-option-value="0">

Here you set a data-property to link this li back to the option element with that value. This gets you out of the world of having to build id's via concatenation or split apart id strings to get target option you are interested in. This provides two-way link between these elements. So in the option context you could do something like:

// get target li from selected option
// here this would refer to option
// optVal would take place of your current rcId variable
var optVal = $(this).val();

// in addValues you would do this:
$('<li/>', {
    'class': 'list-group-item justify-content-between',
    'data-option-value': optVal,
    text: rcName
  })

From li context you would determine target option like this:

var hiddenValue = $(this).data('option-value');
$('#select2 option[value="' + hiddenValue + '"]').show();

UPDATE

To answer your question about how to get rid of your anti-pattern around id properties. Consider an <li> element that looks like this:

<li class="list-group-item justify-content-between" data-option-value="0">

Here you set a data-property to link this li back to the option element with that value. This gets you out of the world of having to build id's via concatenation or split apart id strings to get target option you are interested in. This provides two-way link between these elements. So in the option context you could do something like:

// get target li from selected option
// here this would refer to option
// optVal would take place of your current rcId variable
var optVal = $(this).val();

// in addValues you would do this:
$('<li/>', {
    'class': 'list-group-item justify-content-between',
    'data-option-value': optVal,
    text: rcName
  })

From li context you would determine target option like this:

var hiddenValue = $(this).data('option-value');
$('#select2 option[value="' + hiddenValue + '"]').show();
Source Link
Mike Brant
Mike Brant
  • 9.9k
  • 14
  • 24

Consider using array notation for your data properties, as this will clean up your data-value1, data-value2, data-value* anti-pattern and a lot of your javascript code. I would always take a step back and revisit your solution when you find yourself naming variables, element id's, data properties, etc. in this manner. This is almost always an anti-pattern that suggests you should be using an array. That might make your option HTML look like this:

<option value="0" data-values="[1.0,2.0,3.0]" style="display:none;">default</option>

The nice side-benefit to this is that now your solution becomes scalable for any number of data elements.

Of course the above approach has ramifications on your javascript. For example, your addValues() function might need to look like this:

function addValues(rcName, rcValue, dataValues) {
  $li = $('<li/>', {
    'class': 'list-group-item justify-content-between',
    'id': 'groupItem' + rcValue,
    text: rcName
  });    
  dataValues.forEach(function(value) {
    $li.append(
      $('<span/>', {
        'class': 'label label-default',
        text: value
      })
    );
  });
  // not shown - continue building li here

  // append at the end
  $('#tbList').append($li);
}

Again, this code is now scalable for any number of data values (and shorter).

I would also consider de-nesting your li construction from the $('#tbList').append() operation as I have done in the example above. I think this make the whole thing easier to read/understand.

You can probably address the same name* anti-pattern in your HTML element ID's. You probably should not have to worry about dynamically generating id names. Instead, just apply a class to all these elements and use data-properties on the elements to provide references between elements.

You should cache jQuery selectors that you are going to be using repeatedly, rather than repeatedly querying the DOM.

Don't use alert() for debugging (it looks like you did this at one point due to your commented-out line). Use console.log(). This allows you to dump debug information without actually impacting the way the UI works.

Do you really need to attach your onclick handler to <body>? If there is some other common ancestor for all these elements (div.container for example) you should attach this behavior there so you are not checking this event bubbling condition every single time a click happens on the page, even in unrelated areas.