HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Does this method contain multiple implementations?

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
thisimplementationsmethodcontainmultipledoes

Problem

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.

Solution

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.

Context

StackExchange Code Review Q#135266, answer score: 6

Revisions (0)

No revisions yet.