4
\$\begingroup\$

I want to get my code cleanner and more efficient. This code gets variables form a PHP file and filters it to show the selected user name, all available usergroups on a list box and the groups he is currently into in another.

<script>
    
    function saveGroup(type,id){
      $('#useridgr').attr('placeholder', '').val('');
      $("#usernamegr").attr('placeholder', 'Nome').val('');
      $('#list_box').html('');
      $('#list_box_ini').html('');
    
      if (type == 1){
    
    
      } else {
        loadatagroup(id);
      }
    
    }
    var loadatagroup = (uid) => {
    
      $.ajax({
        type: 'GET',
        url: 'usergrupo/' + uid,
        data: {'_token': $("input[name='_token']").val()},
        success: function(data){
    
    
          console.log("success");
    
          var o = data[0];
    
          $('#useridgr').attr('placeholder',o.id).val(o.id);
          $("#usernamegr").attr('placeholder',o.name).val(o.name);
    
          console.log('areaa');
    
          var size = Object.size(data);
    
    
    
    
          console.log(size);
    
    
          for (var i=0; i<size; i++){
            var o = data[i];
            $("#list_box").append("<option value='"+o.idgrupo+"'>"+o.nome+"</option>");
          }
    
     
        }
    
      })
    
    }
    
    Object.size = function(obj) {
      var size = 0, key;
      for (key in obj) {
        if (obj.hasOwnProperty(key)) size++;
      }
      return size;
    };
    
    </script>
\$\endgroup\$

2 Answers 2

1
\$\begingroup\$

Since you are using jquery, you can use its built in each function to loop through the json results, which will eliminate the need for the size function.

i = index, v = values

$.each(data,function(i,v){
   $("#list_box").append("<option value='"+v.idgrupo+"'>"+v.nome+"</option>");
})
\$\endgroup\$
1
\$\begingroup\$

.Naming variables / constants properly would make your code more readable

var o = data[0];
// becomes
const firstuser = data[0];

Magic numbers should be converted into named constants.

if (type == 1){
    
    
} else {
    loadatagroup(id);
}
// becomes
if (type!==TYPE_DONT_LOAD) loaddatagroup(id);

You mentioned efficiency. You're querying the DOM a lot. Since presumably none of these elements get removed, you can simply hold a reference to them for later.

const userIdText = document.getElementById('userIdText');
const userNameText = document.getElementById('userIdText');
const listBox = document.getElementById('list-box');

The entire Object.size polyfill is unecessary, as you just end up iterating over an array.

The AJAX call requires you to use a callback function. ES6 Javascript provides a nicer alternative to this in the form of async/await and fetch().

const userIdText = document.getElementById('userIdText');
const userNameText = document.getElementById('userIdText');
const listBox = document.getElementById('list-box');

function createGroupOption(usergroup) {
    const option = document.createElement('option');
    option.value = usergroup.idgrupo;
    option.innerText = usergroup.name;
    return option;
}

function updateDisplay(usergroups) {
    const firstuser = usergroups[0] || {id:'',name:'Nome'};
    userIdText.value = firstuser.id;
    userNameText.value = firstuser.name;

    listBox.innerHTML='';
    const options = usergroups.map(createGroupOption);
    options.forEach(option=>{
        listBox.appendChild(option)
    });
}

const TYPE_DONT_LOAD = 1;
function saveGroup(type,uid) {
    updateDisplay([]);
    if (type!==TYPE_DONT_LOAD)
        loadGroup(uid);
}

async function loadGroup(uid) {
    const token = $("input[name='_token']").val()
    const response = await fetch(`userGrupo/${uid}?_token=${token}`);
    const usergroups = await response.json();

    updateDisplay(usergroups);
}
\$\endgroup\$

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