current community

your communities

more stack exchange communities

Stack Exchange
tour help
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.
up vote 3 down vote favorite
2

I've made the following and have a few questions:

  1. Do I have some (big) mistakes in this approach? (bad practices, 'this code is trash'...)

    1.1 If I do, can you suggest what to fix?

  2. Are appended element created in good way?
  3. Code in success function seems to be a little long. Any way to refactor?

enter image description here

CSS code

.no-display-table {
    display: none;
}

HTML code

<div class="row">
    ...
    <!-- empty div will hold <select> which will be crated on ajax success --> 
    <div class="append-select col-md-3"></div>
    ...
</div>
...
<!-- onLoad => 'display: none', <tr> will be dynamically crated on Ajax Success -->
<div class="row">
    <div class="col-md-4 col-md-offset-2">
      <table id="options-table" class="table table-striped">
        <thead> <tr><th> Selected options </th></tr> </thead>
        <tbody></tbody>
      </table>
    </div>
</div>

JavaScript code

$(function() {
    $('#options-table').addClass('no-display-table');

    $('#product-option-select').change(function() {
        if($('.append-select select').length > 0) 
            $('.append-select select').remove();

        var selectedOptionId = $(this).val();

        $.ajax({
            type: 'get',
            url: '../option_types/' + selectedOptionId,
            success: function(data) {
                var element = '<select id="value-select" class="form-control">';
                element += '<option selected disabled>Select value...</option>';

                for(var i=0; i<data.length; i++) {
                    element += '<option value="' + data[i].id + '">' + data[i].name + '</option>';
                }

                element += '</select>';
                $('.append-select').append(element);

                $('.append-select').on('change', '#value-select', function() {
                    $('#options-table').removeClass('no-display-table');

                    var selectedValue = $('#value-select option:selected').text();
                    var element = '<tr><td>' + selectedValue + '</td>';
                        element += '<td><a class="remove"> x </a></tr>';

                    $('#options-table tbody').append(element);

                    $('#options-table').on('mouseover', '.remove', function() {
                        $('.remove').css('cursor', 'pointer');
                    });

                    $('#options-table').on('click', '.remove', function() {
                        $(this).parent().parent().remove();

                        if($('#options-table tbody tr').length == 0)
                            $('#options-table').addClass('no-display-table');
                    });
                });
            }
        });
    });
});
share|improve this question

1 Answer 1

up vote 2 down vote accepted

My 2 cents:

  • You could use $().hide() instead of assigning your custom hiding css class

  • Similarly you could use $().show() instead of removing your custom hiding css class

  • You do not have to check the length of a jQuery query, just call remove(), it will work.

  • Your building of html inside the success callback is ugly, have a helper function to construct it and consider using DOM functions instead of HTML building. I personally tend to show/hide the select element and then rebuild the list with this helper function:

function setOptions( id , array )
{
  //Get the element
  var select = document.getElementById( id ) , i 
  //Remove potentially existing children, somewhat heartless
  while (select.firstChild)
    select.removeChild(select.firstChild);
  //Add the new children
  for( i = 0 ; i < array.length ; i++ )
  {
    var element = new Option( array[i].id , array[i].name );
    select.appendChild(element);        
  }      
}

  • Assiging the listener in the success callback is also too much, consider building a listener that takes into account new elements on document instead $(document').on('change', etc.

  • Same for the mouseover / click event listeners

Simply having the document element ( or any other parent element that stays in the DOM ) and the selector inside the on call should activate the listeners for new elements.

function initializeListeners()
{
  $(document).on('change', '#value-select', function() {
      $('#options-table').removeClass('no-display-table');

      var selectedValue = $('#value-select option:selected').text();
      var element = '<tr><td>' + selectedValue + '</td>';
          element += '<td><a class="remove"> x </a></tr>';

      $('#options-table tbody').append(element);

      $('#options-table').on('mouseover', '.remove', function() {
          $('.remove').css('cursor', 'pointer');
      });

      $('#options-table').on('click', '.remove', function() {
          $(this).parent().parent().remove();

          if($('#options-table tbody tr').length == 0)
              $('#options-table').addClass('no-display-table');
      });
  });
}
share|improve this answer
    
Assiging the listener in the success callback is also too much....Same for the mouseover / click event listeners Can you please provide code for that? –  Srle Dec 20 '13 at 19:02

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.