Validating multiple EditText fields

Question

I am using the following method to validate if user has given any input or left an EditText empty. I am concerned about the return statement in particular, because my IDE keeps complaining about it.

Environment: Android Studio 1.0

public void clientsideauth() {

        if (TextUtils.isEmpty(edt_name.getText().toString().trim())) {
            onShake(edt_name, "Please enter a valid name");
            return;
        } else {
            name = edt_name.getText().toString().trim();
        }
        if (isValidEmail(edt_email.getText().toString().trim())) {
            email = edt_email.getText().toString().trim();
        } else {
            onShake(edt_email, "Please enter a valid email ID");
            return;
        }

        if (((edt_password.getText().length() < 6) || (TextUtils.isEmpty(edt_password.getText().toString().trim())))) {
            onShake(edt_password, "Password must be at least six characters long");
            return;
        } else {
            pwd = edt_password.getText().toString().trim();
        }

        if (TextUtils.isEmpty(edt_password1.getText().toString().trim())) {
            onShake(edt_password1, "Not valid");
            return;
        } else {
            pwd1 = edt_password1.getText().toString().trim();
        }


        if (pwd.equals(pwd1)) {
            startbackgroundtask("formBased");
        } else {
            onShake(edt_password, "Passwords do not match");
            onShake(edt_password1, "Passwords do not match");
            return;
        }
    }

The return type of the method is void, because I don't want any return from this, I only want to stop execution if validation fails. However my IDE complains about the last return statement in the last if-else construct. I am wondering if I should in fact change the return type from void to boolean and maintain an array value for each form field. Later on I can check if all the array values are true or not.

The above function gets called on the Submit button and is further processed to the server.

Answer 1

The warning is trying to tell you that the return is pointless, because it's the last statement anyway. Here's a better way to rewrite that, which will make the warning naturally go away:

if (!pwd.equals(pwd1)) {
    onShake(edt_password, "Passwords do not match");
    onShake(edt_password1, "Passwords do not match");
    return;
} 

startbackgroundtask("formBased");

I simply inverted the condition, and removed the else branch. If you think about it, all the else branches are unnecessary when the if branch returns. You can apply this to all the conditions in the posted code.

---

Many of your conditions handle the invalid case in the if and the normal case in the else, but not all. It would be better to consistently handle the invalid case first everywhere, and use early returns so you can get rid of the else. Like this:

    if (TextUtils.isEmpty(edt_name.getText().toString().trim())) {
        onShake(edt_name, "Please enter a valid name");
        return;
    }
    name = edt_name.getText().toString().trim();

    if (!isValidEmail(edt_email.getText().toString().trim())) {
        onShake(edt_email, "Please enter a valid email ID");
        return;
    }
    email = edt_email.getText().toString().trim();

    if (((edt_password.getText().length() < 6) || (TextUtils.isEmpty(edt_password.getText().toString().trim())))) {
        onShake(edt_password, "Password must be at least six characters long");
        return;
    }
    pwd = edt_password.getText().toString().trim();

    if (TextUtils.isEmpty(edt_password1.getText().toString().trim())) {
        onShake(edt_password1, "Not valid");
        return;
    }
    pwd1 = edt_password1.getText().toString().trim();

    if (!pwd.equals(pwd1)) {
        onShake(edt_password, "Passwords do not match");
        onShake(edt_password1, "Passwords do not match");
        return;
    }

    startbackgroundtask("formBased");

---

Notice that the code contains many duplicated logic, for example here, the code to extract and clean edt_name is written twice:

    if (TextUtils.isEmpty(edt_name.getText().toString().trim())) {
        onShake(edt_name, "Please enter a valid name");
        return;
    }
    name = edt_name.getText().toString().trim();

It would be better to rewrite without duplication, like this:

    String cleanedName = edt_name.getText().toString().trim();
    if (cleanedName.isEmpty()) {
        onShake(edt_name, "Please enter a valid name");
        return;
    }
    name = cleanedName;

I also removed TextUtils.isEmpty, which was either a bug or pointless:

• It's a bug if edt_name can be null, or if edt_name.getText() might return null: in both of these cases you will get a NullPointerException
• If there won't be nulls, then you don't need TextUtils, you can use isEmpty method of String

---

There are several elements of poor coding style:

• Variable names and method names should be camelCase, not like "edt_name" or "clientsideauth"
• pwd and pwd1 are also poorly named, since pwd is the first password and pwd1 is the second. pwd1 and pwd2, or even pwd and pwdConfirmation would have been even better.

---

The clientsideauth method is doing too much:

• Validate input
• If successful, start background task
• It mutates variables defined outside the method

Instead of mutating variables defined outside, it would be better to validate, collect the cleaned values in local variables, and then pass these variables to the authentication task.

---

Since you do .getText().toString().trim() so often, a helper method to do this for you would be a good idea, to reduce your typing and the noise for readers.

Answer 2

This is what I have come up with:

Package structure:

The Trim class:

public class Trim {

    public static String text(EditText v){
        return v.getText().toString().trim();
    }
}

The EmailValidation class:

public class EmailValidation {

    public static boolean isValidEmail(CharSequence target) {
        return android.util.Patterns.EMAIL_ADDRESS.matcher(target).matches();
    }
}

The modified clientsideauth method:

public void clientSideAuth() {

        String cleanName = Trim.text(edt_name);
        if (cleanName.isEmpty()) {
            ShakeAndReport.onShake(edt_name, "Please enter a valid name", MainActivity.this);
            return;
        }
        //  name = cleanName;

        String cleanEmail = Trim.text(edt_email);
        if (!EmailValidation.isValidEmail(cleanEmail)) {
            ShakeAndReport.onShake(edt_email, "Please enter a valid email ID", MainActivity.this);
            return;
        }
        //   email = cleanEmail;

        String cleanPwd1 = Trim.text(edt_password);
        if ((cleanPwd1.length() < 6) || (cleanPwd1.isEmpty())) {
            ShakeAndReport.onShake(edt_password, "Password must be at least six characters long", MainActivity.this);
            return;
        }
        //   pwd = cleanPwd1;

        String confirmPwd = Trim.text(edt_passwordConfirmation);
        if (confirmPwd.isEmpty()) {
            ShakeAndReport.onShake(edt_passwordConfirmation, "Not valid", MainActivity.this);
            return;
        }
        //  pwdConfirmed = confirmPwd;

        if (!cleanPwd.equals(pwdConfirmed)) {
            ShakeAndReport.onShake(edt_password, "Passwords do not match", MainActivity.this);
            ShakeAndReport.onShake(edt_passwordConfirmation, "Passwords do not match", MainActivity.this);
            return;
        }

        startbackgroundtask("formBased", cleanName, cleanEmail, confirmPwd);

    }