2
\$\begingroup\$

I've been working on an edit/update button that will toggle a modal that looks like this:

Update/Edit Employee Modal

Is there a better code for this JavaScript code that I made?

<script>

$('body').on('click', '.editButton',function(){
    let edit = $(this).val()
    console.log("Edit this : " + edit);

    $('#updateEmpModal').modal('toggle');


    $.ajax({
        type    :   'POST',
        url     :   'get.php',
        data    :   {'edit' : edit},
        dataType:   'json',
        success: function(data){
            $("#updateEmpForm input[name='EMAIL']")         .val(data.EMAIL);
            $("#updateEmpForm input[name='PASSWORD']")      .val(data.PASSWORD);
            $("#updateEmpForm input[name='RIGHTS']")        .val(data.RIGHTS);
            $("#updateEmpForm input[name='LAST_NAME']")     .val(data.LAST_NAME);
            $("#updateEmpForm input[name='FIRST_NAME']")    .val(data.FIRST_NAME);
            $("#updateEmpForm input[name='MIDDLE_NAME']")   .val(data.MIDDLE_NAME);
            $("#updateEmpForm input[name='SUFFIX']")        .val(data.SUFFIX);
            $("#updateEmpForm input[name='GENDER']")        .val(data.GENDER);
            $("#updateEmpForm input[name='BIRTHDATE']")     .val(data.BIRTHDATE);
            $("#updateEmpForm input[name='BIRTHPLACE']")    .val(data.BIRTHPLACE);
            $("#updateEmpForm input[name='CITIZENSHIP']")   .val(data.CITIZENSHIP);
            $("#updateEmpForm input[name='RELIGION']")      .val(data.RELIGION);
            $("#updateEmpForm input[name='ADDRESS']")       .val(data.ADDRESS);
            $("#updateEmpForm input[name='CONTACT']")       .val(data.CONTACT);
            $("#updateEmpForm input[name='ICE_NAME']")      .val(data.ICE_NAME);
            $("#updateEmpForm input[name='ICE_CONTACT']")   .val(data.ICE_CONTACT);
            $("#updateEmpForm input[name='DEPARTMENT']")    .val(data.DEPARTMENT);
            $("#updateEmpForm input[name='POSITION']")      .val(data.POSITION);
            $("#updateEmpForm input[name='EMP_TYPE']")      .val(data.EMP_TYPE);
            $("#updateEmpForm input[name='edit']")          .val(data.ID);
        }
    });
});

</script>

It is working, but I want to know if this is a bad practice and if there are better solutions.

New contributor
Gene Adrian San Luis is a new contributor to this site. Take care in asking for clarification, commenting, and answering. Check out our Code of Conduct.
\$\endgroup\$
  • \$\begingroup\$ Safe to remove the php tag here? \$\endgroup\$ – mickmackusa Apr 16 at 0:31
1
\$\begingroup\$

Welcome to Code Review! Hopefully you enjoy using this site and receive valuable feedback.


Wow, what patience you have to type out/copy all of those lines to update the form inputs. Instead of doing that, you could loop through the input fields and check if the name matches a key in the returned data. One can use .each() to iterate over the input fields, then check the name using .attr() and the in operator.

$('#updateEmpForm input').each(function() {
    const inputName = $(this).attr('name');
    if (inputName in data) {
        $(this).val(data[key]);
    }
});

To exclude certain fields, like any password inputs, the selector could be updated to include pseudo-class selectors like `:not() - for example:

$('#updateEmpForm input:not([type="password"])').each(function() {

That should handle all but the last repeated line - which can be included before or after the call to .each():

$("#updateEmpForm input[name='edit']").val(data.ID);

You could also define a mapping of input names to keys and look in the mapping for values. For example:

const inputNameKeyMapping = {
    edit: 'ID',
    //any other names that don't match keys directly
}

And use that mapping when assigning the value - something like:

$('#updateEmpForm input').each(function() {
    const inputName = $(this).attr('name');
    const key = inputNameKeyMapping[inputName] || inputName;
    if (key in data) {

That way you wouldn't need to include manual value settings lines.


Also, instead the click handler can be simplified from:

$('body').on('click', '.editButton',function(){

To using the .click() (short-cut) method on only elements with that class name editButton:

$('.editButton').click(function(){

I see let is used to declare the variable edit:

let edit = $(this).val()

However, that value is never re-assigned. To avoid unintentional re-assignment, you could use const instead.

const edit = $(this).val()

And it is best to be consistent with line terminators - if most lines have them then make sure all do.

\$\endgroup\$

Your Answer

Gene Adrian San Luis is a new contributor. Be nice, and check out our Code of Conduct.

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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