patterncsharpMinor
nested if structure, how could it be improved?
Viewed 0 times
improvedcouldnestedstructurehow
Problem
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();
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
}
```
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();
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
}
Solution
I would first start by creating a method for the Authentication failed parts of the code. Something like
Then your first switch for the FB authentication ends up being something like:
I would further refactored the getting of user email into it's own method as that seems to be common across both forms.
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();
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.
{
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();
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.
{
Code Snippets
private AuthenticationResult AuthenticationFailed(AuthenticationStatus status, string message = "", Exception exception = null)
{
return new AuthenticationResult
{
Status = status,
Message = message,
Exception = exception
}
}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);
}
}
}private User GetUserByEmail(string email)
{
return email.NullOrEmpty() ?? null : userRepository.GetByEmail(email);
}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.
Context
StackExchange Code Review Q#13447, answer score: 5
Revisions (0)
No revisions yet.