Returning different types from a C# method

Question

I have a method:

public ??? AuthManager.Login(Credentials credentials)

Here is a set of valid output values of this method:

1. Success (+accountId)
2. Failure: AccountLockedOut
3. Failure: UsernameNotFound
4. Failure: InvalidPassword (+failed attempt count)

Depending on the return type different views are shown to the user (yes, view for AccountLockedOut is different from InvalidPassword).

I could go with:

public class LoginAttemptResult {
    public bool Succeeded { get; set; }
    public AccountId AccountId { get; set; } // for when success
    public LoginAttemptResultEnumType Result { get;set; } // Success, Lockedout, UsernameNotFound, InvalidPassword  
    public int FailedAttemptCount { get; set; } // only used for InvalidPassword
}

I don't like this and looking for a better solution. First, this results in a partially initialized object, two it violates interface segregation principle, three it violates SRP.

UPDATE: throwing exceptions is also not an elegant solution because InvalidPassword as I see it is not an exception. Failed DB connection is an exception. Null argument is an exception. InvalidPassword is a valid anticipated response.

I think better solution is to create a hierarchy of classes:

abstract class LoginAttemptResult
    sealed class LoginSuccess : LoginAttemptResult { AccountId }
    abstract class LoginFailure : LoginAttemptResult
        sealed class InvalidPasswordLoginFailure : LoginFailure { FailedAttemptCount }
        sealed class AccountLockedoutLoginFailure : LoginFailure

the caller of Login method then would have to do something like:

if (result is LoginSuccess) { 
    ..."welcome back mr. account id #" + (result as LoginSuccess).AccountId
}
else if (result is InvalidPasswordLoginFailure ) { 
    ..."you failed " + (result as InvalidPasswordLoginFailure).FailedAttemptCount + " times"
}

I don't see anything wrong (conceptually) with this approach (other than a number of classes it comes with).

What else is wrong with this approach?

Notice, this approach is essentially an F#'s discriminated union (DU) .

Is there a better way to model this? I already have several solutions that work - now I want an elegant solution that works.

Answer 1

Here is a solution that satisfies all my requirements (readability, testability, discoverability and esthetics).

Code (notice the implementation is little different from original task, but concept remains):

public class AuthResult {
    // Note: impossible to create empty result (where both success and failure are nulls).
    // Note: impossible to create an invalid result where both success and failure exist.
    private AuthResult() {}
    public AuthResult(AuthSuccess success) {
        if (success == null) throw new ArgumentNullException("success");
        this.Success = success;
    }
    public AuthResult(AuthFailure failure) {
        if (failure == null) throw new ArgumentNullException("failure");
        this.Failure = failure;
    }
    public AuthSuccess Success { get; private set; }
    public AuthFailure Failure { get; private set; }
}

public class AuthSuccess {
    public string AccountId { get; set; }
}

public class AuthFailure {
    public UserNotFoundFailure UserNotFound { get; set; }
    public IncorrectPasswordFailure IncorrectPassword { get; set; }
}

public class IncorrectPasswordFailure : AuthResultBase {
    public int AttemptCount { get; set; }
}

public class UserNotFoundFailure : AuthResultBase {
    public string Username { get; set; }
}

Notice how AuthResult correctly models a heterogeneous and hierarchical nature of the function range.

And if you add a following implicit operator:

public static implicit operator bool(AuthResultBase result) {
    return result != null;
}

you can use result as follows:

var result = authService.Auth(credentials);
if (result.Success) {
    ...
}

which reads (arguably) better than:

if (result.Success != null) {
    ...
}

Answer 2

Summary: instead of returning a value and decoding it - give Login a set of handlers so Login will call appropriate callback (think jQuery's ajax { success: ..., error: ... })

The consumer of the Login method will have to decode a response using essentially a switch statement. One way to refactor this code to eliminate that "switch" statement and also remove explosion of custom types is instead of asking Login method to return a discriminated union - we give Login method a set of thunks - one for each response.

(subtle point) Technically we don't get rid of custom classes, we simply replace them with generics, i.e. we replaced InvalidPasswordFailedLogin { int failedAttemptCount } with Action<int>. This approach also presents some interesting opportunities, for example Login can be handled async'ly more naturally. Testing on the other hand becomes little more obscure.

public class LoginResultHandlers {
    public Action<int> InvalidPassword { get; set; }
    public Action AccountLockedout { get; set; }
    public Action<AccountId> Success { get; set; }
}

public class AccountId {}

public class AuthManager {
    public void Login(string username, string password, LoginResultHandlers handler) {
        // if (...
            handler.Success(new AccountId());
        // if (...
            handler.AccountLockedout();
        // if (...
            handler.InvalidPassword(2);
    }
}

public class Application {
    public void Login() {
        var loginResultHandlers = new LoginResultHandlers {
                AccountLockedout = ShowLockedoutView,
                InvalidPassword = (failedAttemptCount) => ShowInvalidPassword(failedAttemptCount),
                Success = (accountId) => RedirectToDashboard(accountId)
        };
        new AuthManager().Login("bob", "password", loginResultHandlers);
    }

    private void RedirectToDashboard(AccountId accountId) {
        throw new NotImplementedException();
    }

    private void ShowInvalidPassword(int failedAttemptCount) {
        throw new NotImplementedException();
    }

    private void ShowLockedoutView() {
        throw new NotImplementedException();
    }
}

Answer 3

your security api should not be exposing so much information. The API you posted provides no useful information to a client, other than to aid an attacker in trying to hijack an account. Your login method should provide only pass/fail information and the token that can be passed on to any authorization mechanism you need.

// used by clients needing to authenticate
public interfac ISecurity {
  AuthenticationResponse Login(Credentials credentials);
}

// the response from calling ISecurity.Login
public class AuthenticationResponse {

  internal AuthenticationResponse(bool succeeded, AuthenticationToken token, string accountId) {
    Succeeded = succeeded;
    Token = token;
  }

  // if true then there will be a valid token, if false token is undefined
  public bool Succeeded { get; private set; }

  // token representing the authenticated user.
  // document the fact that if Succeeded is false, then this value is undefined
  public AuthenticationToken Token { get; private set; }

}

// token representing the authenticated user. simply contains the user name/id
// for convenience, and a base64 encoded string that represents encrypted bytes, can
// contain any information you want.
public class AuthenticationToken {

  internal AuthenticationToken(string base64EncodedEncryptedString, string accountId) {
    Contents = base64EncodedEncryptedString;
    AccountId = accountId;
  }

  // secure, and user can serialize it
  public string Contents { get; private set; }

  // used to identify the user for systems that aren't related to security
  // (e.g. customers this user has)
  public string AccountId { get; private set; }

}


// simplified, but I hope you get the idea. It is what is used to authenticate
// the user for actions (i.e. read, write, modify, etc.)
public interface IAuthorization {
  bool HasPermission(AuthenticationToken token, string permission); 
}

You will notice that this API does not have log in attempts. The client should not care about the rules involved with logging in. The implementer of the ISecurity interface should keep up with log in attempts, and return fail when a successful set of credentials has been passed in, but the number of attempts has been exceedeed.

A simple message on failure should read something along the lines of:

Could not log you on at this time. Check that your username and/or password are correct, or please try again later.

Answer 4

Another possible approach is to create a class that encapsulates the Login process and its results, like this:

    public interface ILoginContext
    {
        //Expose whatever properties you need to describe the login process, such as parameters and results

        void Login(Credentials credentials);
    }

    public sealed class AuthManager
    {
        public ILoginContext GetLoginContext()
        {
            return new LoginContext(this);
        }

        private sealed class LoginContext : ILoginContext
        {
            public LoginContext(AuthManager manager)
            {
                //We pass in manager so that the context can use whatever it needs from the manager to do its job    
            }
            //...
        }
    }

Basically what this design implies is that logging in has become a complex enough operation that a single method is no longer an appropriate encapsulation. We need to return a complex result and might want to include more complex parameters. Because the class is now responsible for the behavior and not just representing data, it's less likely to be considered a violation of SRP; it's just a somewhat complex class for a somewhat complex operation.

Note that you might also make the LoginContext implement IDisposable if it has a natural transactional scope.

Answer 5

I think your solution is OK in the case if result classes differs significantly and you need a separate class for each. But I'm not sure about that. Try this class for each result:

/// <summary>
/// Immutable, created by the server
/// </summary>
class LoginResult
{
    /// <summary>
    /// Null in the case of failure
    /// </summary>
    public int? Id { get; private set; }

    /// <summary>
    /// Null in the case of success
    /// </summary>
    public string FailReason { get; private set; }

    /// <summary>
    /// Always >= 1
    /// </summary>
    public int AttemptNumber { get; private set; }

    public LoginResult(int id, int attemptNumber)
    {
        Id = id;
        AttemptNumber = attemptNumber;
    }

    public LoginResult(string reason, int attemptNumber)
    {
        FailReason = reason;
        AttemptNumber = attemptNumber;
    }
}

I can imagine, that your authentication logic can be very complicated, and Id, FailReason and AttemptNumber are not only properties you'll need. In this case you need to present us more concrete example, we'll try to build abstractions that will fit your logic, if neccessary. In this particular case - no sense for abstraction.

Answer 6

If you make your LoginAttemptResult class abstract, then you can add an abstract property Message that will force your child classes to implement it.

public abstract class LoginAttemptResult
{        
    public abstract string Message { get; }

    // any other base methods/properties and abstract methods/properties here
}

Then, your children could look like this:

public class LoginSuccess : LoginAttemptResult
{
    public override string Message 
    { 
        get
        {
            return "whatever you use for your login success message";
        }
    }
}

With that, your Login method could just return a LoginAttemptResult

public LoginAttemptResult AuthManager.Login(Credentials credentials)
{
    // do some stuff
}

And then your caller would just call your LoginAttemptResult.Message (or whatever other things you needed it to do):

var loginResult = AuthManager.Login(credentials);
var output = loginResult.Message;

Similarly, if you needed to have some other method associated with your LoginAttemptResult based on the child type, you could define it as an abstract method in your base class, implement it in your child classes, and then call it the exact same way.

Answer 7

You could make return a Tuple

public Tuple<T1,T2> AuthManager.Login(Credentials credentials){
//do your stuff here
return new Tuple<T1,T2>(valueOfT1,valueOfT2);
}