How can I simplify this JQuery validation process?

Question

How can I simplify this functioning JQuery validation process:

class UserEdit {
  addUser(uobj) {
    if(uobj.adduser == "" && uobj.addfullname == "" && uobj.addemail == "" && uobj.addlevel == "")
    {
        $('#adduserError').show();
        $('#addfullnameError').show();
        $('#addemailError').show();
        $('#adduserlevelError').show();
        return false;
    }
    if(uobj.adduser == "")
    {
        $('#adduserError').show();
        return false;
    }
    if(uobj.addfullname == "")
    {
        $('#addfullnameError').show();
        return false;
    }
    if(uobj.addemail == "" || (uobj.addemail.indexOf("@",0)) == -1 || (uobj.addemail.indexOf(".",0)) == -1 ||  uobj.addemail.length < 6)
    {
        $('#addemailError').show();
        return false;
    }
    else
    {
      $('#loadingDiv').show();
      $.post('process/editUser.php', {uobj:uobj}, function(data)
      {
        var modal = "#addUserModal";
        $('#loadingDiv').hide();
        dataCheck(data, modal);
      });
    }
  }
}

All of the above works as it should. I just know there must be a way to simplify it.

Note (because I believe someone will ask): I use the dataCheck function to check the data returned from the PHP process, and then close the modal if the process was successful. There are a couple of other modules within the class that use the same dataCheck function. That was my initial attempt to at least simplify how to handle the return data in each module.

Now I want to try to simply the code above. Any help is appreciated.

Answer 1

I see you removed the calls to $('#loadingDiv').show(); from the many conditional blocks and moved that before the call to the AJAX call - I was going to suggest that.

I see the ecmascript-6 features like the Class syntax are used, so other ES-6 features could be used - like the object initializer shorthand notation - i.e. on this line:

 $.post('process/editUser.php', {uobj:uobj}, function(data)

The second argument can be simplified:

$.post('process/editUser.php', {uobj}, function(data)

There is no need to use the else keyword since all previous conditional blocks have a return statement.

The callback to the AJAX call has a variable modal:

var modal = "#addUserModal";

The variable is only used one time so the value can be used in the call to dataCheck and the variable eliminated. Additionally, unless there is a good reason like needing a global variable, don't use var. Use const and then if you determine re-assignment is necessary use let.

Instead of using .indexOf() to determine if a string does or does not contain a substring, String.prototype.includes() could be used unless browser compatibility is an issue.

I know that the PHP PSRs recommend having opening braces on a new line but this is less common in JavaScript.

Answer 2

Return Value

The method addUser returns false for all negative cases. Below it is possible to see that it does not return true for the positive case.. instead it returns nothing which leads to an undefined:

else
{
 $('#loadingDiv').show();
 $.post('process/editUser.php', {uobj:uobj}, function(data)
 {
   var modal = "#addUserModal";
   $('#loadingDiv').hide();
   dataCheck(data, modal);
 });
}

This makes the method hard to understand when working with it without knowing the implementation.

---

Concerns

The class UserEdit handles multiple concerns. There are two principles which can be named to emphasize the importance for separation:

• Separation of Concerns (SoC)
• Single Responsibility Principle (SRP)

The concerns are

• validating (uobj.adduser == "")
• presenting ($('#adduserError').show();)
• sending data ($.post(...))

When we separate the concerns we could accomplish something like

class UserEdit {
    constructor(validator, presenter, repository) {/* ... */}

    addUser(user) {
      const validation = validator.validate(user);
      presener.present(validation);
      
      if (validation.isValid())
          repository.update(user);

      return validation.isValid();
    }
}

This has multiple advantages. Beside the descriptive and shorter method body we can accomplish through the dependency injection via the constructor a better way to test the class. Additionally the modification and the exchange of the modules is easier.