3

I have a method login() which essentially sends a login request to a remote server. As I do not want to waste server resources on processing invalid data, I decided to consider sanity checking the input before I send it.

The code looks something this:

//Method to log user in
public void login(String username, String password) throws IOException{
    //If statement to sanity check credentials
    if (credentialsSanityCheck(username, password)) {
        //Perform login ...
    }
    else {
        throw new IllegalArgumentException("Invalid username/password");
    }
}

However, after learning more about the Single Responsibilty Principle, I am now wondering if it is more appropriate to sanity check the method arguments from the calling code or within the method itself, as it's quite fuzzy (at least for me) to determine whether or not data validation falls under the method's responsibility.

  • 1
    Not really related to SRP but you always need to validate on the server-side. You can't control the client and you can't even be sure that a client you have created is sending requests to the server. Are you thinking you wouldn't validate at all on the server side? – JimmyJames Feb 4 at 17:09
  • @Jimmy It's more of an additive measure, validation is also occurring at the server and at the database – Garikai Feb 4 at 17:12
  • What layer of your architecture is is this code? – Greg Burghardt Feb 4 at 18:03
7

Aspects are odd ducks in SRP (security, logging, etc.). They are often required for a variety of truly valid business reasons, but seem to push a lot of conditional steps in (if the input is safe, then use it to log in, and if something weird happens, log it).

Depending on how pedantic you are, sure, it violates SRP. But, to paraphrase, SRP is not a suicide pact. A somewhat more palatable construction might be making sure both bodies in the conditional are at the same level of abstraction:

//Method to log user in
public void login(String username, String password) throws IOException{
   //If statement to sanity check credentials
   if (credentialsSanityCheck(username, password)) {
      loginWithSafedInput(username, password); // might fail due to user error, locked acct, etc... 
   }
   else {
      raiseUnsafeLogin(username, password); // audit trail and throws
  }
}

The point is to safely log in, with forensic evidence if needed. That seems SRP'y, if you squint.

  • Seems like a fair compromise – Garikai Feb 4 at 20:19
3

The login function has one single responsibility: log you in safely, or reject you.

“Safely” is part of its responsibility. That cannot be left to the caller. If you do checks outside the call, login must repeat these checks or it doesn’t even achieve its own responsibility.

SRP is the one if the most misinterpreted principles. You call this “A fair compromise” in a comment. Wrong. SRP doesn’t apply here at all. It often doesn’t apply at all when people think it does who have just learned about it.

  • I agree that SRP doesn't mean literal single responsibility, or most side effects (logging) would be right out. The idea is to have a cohesive task. Crosscutting concerns are tough to handle elegantly regardless of mental framework. Without crosscut support in the language (or an available framework such as AspectJ), I accept compromise is a fair description because your are still trying to make cohesive the Login Process with the Safe To Proceed. I'd probably have tried to sanitize the input, and then login to separate the concerns / implementations. – Kristian H Feb 5 at 18:31

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.