5
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Do you want to know what it does or how to make it better? It reads like you're posting 2 questions here and only one of them is on-topic. \$\endgroup\$ – Mast Jul 19 '16 at 10:03
  • \$\begingroup\$ Well, I would like to know if validateUsername contains multiples responsibilities or not. If it is true, I would like to know how improve it with a clean solution. \$\endgroup\$ – juanjo Jul 19 '16 at 10:47
6
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ validateUsername method notifies to the view that one error happened. => View.setUsernameModeError(validationResult.getMessage()); \$\endgroup\$ – juanjo Jul 19 '16 at 10:51
1
\$\begingroup\$

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.

\$\endgroup\$
0
\$\begingroup\$

"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.

\$\endgroup\$

Your Answer

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.