Does this method contain multiple implementations?

Question

My teammate and I have a small discussion about the responsibility of a method. He thinks next method has two responsibilities and I think that only one.

 private boolean validateUsername(String username) {
    AccountValidationResult validationResult =
            mAccountValidator.validate(username, new int[]{AccountValidation.VALIDATION_REQUIRED});

    if (!validationResult.isSuccessful()) {
        mView.setUsernameModeError(validationResult.getMessage());
    } else {
        mView.setUsernameModeStandard();
    }
    return validationResult.isSuccessful();
}

I think that the responsibilities of validating the string "username" falls on the "validate" method of my mAccountValidator collaborator. Depending of the result returned by my collaborator, I change the state of the view and finally, in order to finish the process or not, I return the state of validation. This result is managed by the next method:

public void doLogin(UserCredentialModel userCredential) {

    if (!validateUsername(userCredential.getUsername())) {
        return;
    }

    if (!validatePassword(userCredential.getPassword())) {
        return;
    }

    mDoLogin.setUserCredential(userCredential);
    mDoLogin.execute();
}

What do you think about my solution? Do you think that is it enough clean or how I could improve it?

Thank a lot for your time and help.

Answer 1

Seemed fine at first, then I took another look...

The validation function displays the error message?

I'm sorry, but that's indeed a violation of SRP. You'd be better off returning a ValidationResult which you could use. You already HAVE this validation result... so in a way, you've already done what I said.

What's wrong is that your function names do not reflect this. You call validateUsername, but it also displays errors. Rather than changing the validation method to return a ValidationResult (which would imply taking out validateUsername and doing everything in doLogin, you should change the function name instead.

Answer 2

After reading the code, I came to the same conclusion that @Pimgd did, that validateUsername seemed like the wrong name for the method.

This isn't really what you were after, but this:

AccountValidationResult validationResult =
        mAccountValidator.validate(username, 
                                   new int[]{AccountValidation.VALIDATION_REQUIRED});

Seemed odd to me. Obviously I've not seen the contents of AccountValidation or your validators, but it seems odd to me to tell a validator to validate and still have to pass it an argument that contains a parameter VALIDATION_REQUIRED. It does make you wonder what else is going on there.

Answer 3

"I change the state of the view AND ... I return the state of the validation."

You already give yourself the answer: Two responsibilities.

This is not because of that you combine UI issues and validation logic in one method. It is because of "and". Effectively you have two results out of one method. And this is a violation of SRP by itself.

The more important thing is: you coupled things that should be separated. The UI should be informed in an abstract way, for example with an observer pattern.