nested if structure, how could it be improved?

Question

I feel like this structure could be better, but I can't realize how.

    internal AuthenticationResult ProcessOpenIdResponse(IAuthenticationResponse response)
    {
        switch (response.Status)
        {
            case AuthenticationStatus.Authenticated:
            {
                string openId = response.ClaimedIdentifier;
                User user = userRepository.GetByOpenId(openId);
                bool userIsNew = false;
                bool connectionIsNew = false;
                if (user == null)
                {
                    connectionIsNew = true;

                    string email = null;
                    string displayName = null;
                    var fetch = response.GetExtension<FetchResponse>();
                    if (fetch != null)
                    {
                        email = fetch.GetAttributeValue(WellKnownAttributes.Contact.Email);
                        displayName = fetch.GetAttributeValue(WellKnownAttributes.Name.FullName);
                    }
                    if (!email.NullOrEmpty()) // maybe they already have an account.
                    {
                        user = userRepository.GetByEmail(email);
                    }
                    if (user == null) // create a brand new account.
                    {
                        user = userRepository.CreateFromOpenId(openId, email, displayName);
                        userIsNew = true;
                    }
                    else // just connect the existing account to their OpenId account.
                    {
                        userRepository.AddOpenIdConnection(user, openId);
                    }
                }
                return new AuthenticationResult
                {
                    Status = ConnectionStatus.Authenticated,
                    UserIsNew = userIsNew,
                    ConnectionIsNew = connectionIsNew,
                    DisplayName = user.DisplayName,
                    UserId = user.UserId
                };
            }
            case AuthenticationStatus.Canceled:
            {
                return new AuthenticationResult
                {
                    Status = ConnectionStatus.Canceled,
                    Message = Common.Resources.Authentication.CanceledAtProvider
                };
            }
            case AuthenticationStatus.Failed:
            {
                return new AuthenticationResult
                {
                    Status = ConnectionStatus.Faulted,
                    Message = response.Exception.Message,
                    Exception = response.Exception
                };
            }
            default:
            {
                return new AuthenticationResult
                {
                    Status = ConnectionStatus.Faulted
                };
            }
        }
    }

and another:

    public AuthenticationResult AuthenticateWithFacebook(string facebookId, string accessToken)
    {
        if (facebookId == null)
        {
            throw new ArgumentNullException("facebookId");
        }
        if (accessToken == null)
        {
            throw new ArgumentNullException("accessToken");
        }
        dynamic response;
        try
        {
            FacebookClient client = new FacebookClient(accessToken);
            response = client.Get("me");
        }
        catch (FacebookApiException exception)
        {
            _log.Info(Common.Resources.Error.FacebookApiException, exception);
            return new AuthenticationResult
            {
                Status = ConnectionStatus.Faulted,
                Message = exception.Message,
                Exception = exception
            };
        }
        if (response == null) // sanity
        {
            return new AuthenticationResult
            {
                Status = ConnectionStatus.Faulted,
                Message = Common.Resources.Error.FacebookApiException
            };
        }
        else if (response.error != null)
        {
            return new AuthenticationResult
            {
                Status = ConnectionStatus.Faulted,
                Message = response.error.message ?? Common.Resources.Error.FacebookApiException
            };
        }
        else if (response.id != facebookId) // validate access token against facebookId for enhanced security.
        {
            return new AuthenticationResult
            {
                Status = ConnectionStatus.InvalidCredentials,
                Message = Common.Resources.Authentication.InvalidCredentials
            };
        }
        else
        {
            User user = userRepository.GetByFacebookGraphId(facebookId);
            bool userIsNew = false;
            bool connectionIsNew = false;
            if (user == null)
            {
                connectionIsNew = true;

                string email = response.email;
                if (!email.NullOrEmpty()) // maybe they already have an account.
                {
                    user = userRepository.GetByEmail(email);
                }
                if (user == null) // create a brand new account.
                {
                    string displayName = response.name;
                    user = userRepository.CreateFromFacebook(facebookId, accessToken, email, displayName);
                    userIsNew = true;
                }
                else // just connect the existing account to their Facebook account.
                {
                    userRepository.AddFacebookConnection(user, facebookId, accessToken);
                }
            }
            return new AuthenticationResult
            {
                Status = ConnectionStatus.Authenticated,
                UserIsNew = userIsNew,
                ConnectionIsNew = connectionIsNew,
                DisplayName = user.DisplayName,
                UserId = user.UserId
            };
        }
    }

Just in case anyone wonders about dependencies from outside the method, here is the class:

public class AuthenticationService
{
    private static readonly ILog _log = LogManager.GetLogger(typeof(AuthenticationService));

    private readonly OpenIdRelyingParty relyingParty;
    private readonly IUserRepository userRepository;
    private readonly IMembershipProvider membershipProvider;

    public AuthenticationService(OpenIdRelyingParty relyingParty, IUserRepository userRepository, IMembershipProvider membershipProvider)
    {
        if (relyingParty == null)
        {
            throw new ArgumentNullException("relyingParty");
        }
        if (userRepository == null)
        {
            throw new ArgumentNullException("userRepository");
        }
        if (membershipProvider == null)
        {
            throw new ArgumentNullException("membershipProvider");
        }
        this.relyingParty = relyingParty;
        this.userRepository = userRepository;
        this.membershipProvider = membershipProvider;
    }

    [...]
}

Answer 1

I would first start by creating a method for the Authentication failed parts of the code. Something like

private AuthenticationResult AuthenticationFailed(AuthenticationStatus status, string message = "", Exception exception = null)
{
   return new AuthenticationResult
   {
       Status = status,
       Message = message,
       Exception = exception
   }
}

Then your first switch for the FB authentication ends up being something like:

    internal AuthenticationResult ProcessOpenIdResponse(IAuthenticationResponse response)
    {
        switch (response.Status)
        {
            case AuthenticationStatus.Authenticated:
            {
                // successfull authentication
            }
            case AuthenticationStatus.Canceled:
            {
                return AuthenticationFailed(ConnectionStatus.Canceled, Common.Resources.Authentication.CanceledAtProvider);                                
            }
            case AuthenticationStatus.Failed:
            {
                return AuthenticationFailed(ConnectionStatus.Faulted, response.Exception.Message, response.Exception);                
            }
            default:
            {
                return AuthenticationFailed(ConnectionStatus.Faulted, response.Exception.Message, response.Exception);                
            }
        }
    }

I would further refactored the getting of user email into it's own method as that seems to be common across both forms.

private User GetUserByEmail(string email)
{
    return email.NullOrEmpty() ?? null : userRepository.GetByEmail(email);
}

Although this might be overkill I would also consider separating the methods for doing the authentication into their own classes. I believe this may help with being able to do unit testing against each class in isolation as well as not separating the responsibilities out of whichever class is handling identifying which process to use etc. Something like this perhaps.

abstract class AuthenticationPortal
{   
    abstract AuthenticationResult Authenticate();

    protected virtual User GetUserByEmail(string emailAddress)
    {   
        return emailAddress.NullOrEmpty() ?? null : userRepository.GetByEmail(emailAddress);            
    }

    private AuthenticationResult AuthenticationFailed(AuthenticationStatus status, string message = "", Exception exception = null)
    {
       return new AuthenticationResult
       {
           Status = status,
           Message = message,
           Exception = exception
       }
    }
}

internal class OpenIdAuthenticationPortal : AuthenticationPortal
{
   private readonly IAuthenticationResponse authenticationResponse;
   private readonly UserRepository userRepository;

   public OpenIdAuthenticationPortal(IAuthenticationResponse response, UserRepository userRepository)
   {
      this.authenticationResponse = response;
      this.userRepository = userRepository;
   }

   public AuthenticationResult Authenticate()
   {
        switch (authenticationResponse.Status)
        {
            case AuthenticationStatus.Authenticated:
            {
                return Authenticated();
            }
            case AuthenticationStatus.Canceled:
            {
                return AuthenticationFailed(ConnectionStatus.Canceled, Common.Resources.Authentication.CanceledAtProvider);                                
            }
            case AuthenticationStatus.Failed:
            {
                return AuthenticationFailed(ConnectionStatus.Faulted, authenticationResponse.Exception.Message, authenticationResponse.Exception);                
            }
            default:
            {
                return AuthenticationFailed(ConnectionStatus.Faulted, authenticationResponse.Exception.Message, authenticationResponse.Exception);                
            }
        }
   }

   private AuthenticationResult Authenticated()
   {
        string openId = authenticationResponse.ClaimedIdentifier;
        User user = userRepository.GetByOpenId(openId);
        bool userIsNew = false;
        bool connectionIsNew = false;

        if (user == null)
        {
            connectionIsNew = true;

            string email = null;
            string displayName = null;
            var fetch = authenticationResponse.GetExtension<FetchResponse>();

            if (fetch != null)
            {
                email = fetch.GetAttributeValue(WellKnownAttributes.Contact.Email);
                displayName = fetch.GetAttributeValue(WellKnownAttributes.Name.FullName);

                // try to fetch by email first
                user = GetUserByEmail(userRepository, email);

                if (user == null) // create a brand new account.
                {
                    user = userRepository.CreateFromOpenId(openId, email, displayName);
                    userIsNew = true;
                }
                else // just connect the existing account to their OpenId account.
                {
                    userRepository.AddOpenIdConnection(user, openId);               
                }
            }
        }

        return new AuthenticationResult
        {
            Status = ConnectionStatus.Authenticated,
            UserIsNew = userIsNew,
            ConnectionIsNew = connectionIsNew,
            DisplayName = user.DisplayName,
            UserId = user.UserId
        };
    }
}

The Facebook client would follow similiar pattern.

Answer 2

1. Move common code into helper methods

2. Fail early. Prefer this pattern

   if(something_is_wrong) { throw error }
   

   over

   if(every_is_ok) { ... }
   else { throw error }
   

3. There is little point to use else when the if() before that always throws an error.

4. When a method gets too long, split it into several helper methods with meaningful names. Prefer

   if(!parameter_x_is_valid(x)) { throw "x is invalid" }
   beginTransaction();
   writeToDatabase(x);
   endTransaction();
   

   over one 150 line method where you'll need comments to understand what is happening.

   That will also make it easier to test the code because short helper methods almost always need less setup.