I personally think this is fine if the concept of creating a UserContact, User is directly tied to creating a UserAccount, i.e. one cannot live without the other.
That is because the abstraction of the method CreateAccount() hides away the implementation details of what needs to be done (insertion of data into 3 tables).
As you already have this abstraction you are now in the postion to potentially refactor the CreateAccount without effecting the rest of your code. Excellant!
So, as an option, I might consider refactoring the service layer itself so that the CreateAccount() method is a bit more streamlined, as you put it.
public UserAccount CreateAccount(NewAccount newAccount)
{
if (!AccountAlreadyExists(newAccount.UserName))
{
UserAccount account = CreateUserAccount(newAccount);
User user = CreateUser(newAccount);
UserContact contact = CreateContact(newAccount);
db.Entry(account).State = EntityState.Added;
db.Entry(contact).State = EntityState.Added;
db.Entry(user).State = EntityState.Added;
db.SaveChanges();
return account;
}
return null;
}
private UserAccount CreateUserAccount(NewAccount newAccount)
{
string salt = StringCipher.GenerateCypher();
return new UserAccount()
{
UserId = newAccount.UserName,
PasswordHash = CreatePasswordHash(newAccount.Password),
Lock = true,
Created = DateTime.UtcNow
};
}
private User CreateUser(NewAccount newAccount)
{
return new User()
{
UserId = newAccount.UserName
};
}
private UserContact CreateContact(NewAccount newAccount)
{
return new UserContact()
{
ContactTypeId = ContactType.EmailPrimary,
Contact = StringCipher.Encrypt(newAccount.UserEmail, salt),
UserId = newAccount.UserName,
Active = true,
Created = DateTime.UtcNow
}
}
This first lot of refactoring lead me to realise that newAccount is passed to each of the 3 methods. I thought at this stage perhaps a class for these actions might make sense so that the common parameter is extracted to a class level variable.
public UserAccount CreateAccount(NewAccount newAccount)
{
if (!AccountAlreadyExists(newAccount.UserName))
{
var accountBuilder = new AccountBuilder(newAccount);
Account account = accountBuilder.CreateAccount();
db.Entry(account.UserAccount).State = EntityState.Added;
db.Entry(account.Contact).State = EntityState.Added;
db.Entry(account.User).State = EntityState.Added;
db.SaveChanges();
return account;
}
return null;
}
internal class UserAccountBuilder
{
private readonly NewAccount _newAccount;
public UserAccountBuilder(NewAccount account)
{
_newAccount = account;
}
public Account CreateAccount()
{
return new Account(
CreateUserAccount(),
CreateUser(),
CreateUserContact()
);
}
private string GetUsername()
{
return _newAccount.UserName;
}
private UserAccount CreateUserAccount()
{
string salt = StringCipher.GenerateCypher();
return new UserAccount()
{
UserId = GetUsername(),
PasswordHash = CreatePasswordHash(_newAccount.Password),
Lock = true,
Created = DateTime.UtcNow
};
}
private User CreateUser()
{
return new User()
{
UserId = GetUsername()
};
}
private UserContact CreateContact()
{
return new UserContact()
{
ContactTypeId = ContactType.EmailPrimary,
Contact = StringCipher.Encrypt(_newAccount.UserEmail, salt),
UserId = GetUserName(),
Active = true,
Created = DateTime.UtcNow
}
}
}
public class Account
{
public UserAccount Account { get; private set; }
public User User { get; private set; }
public UserContact Contact { get; private set; }
public Account(
UserAccount userAccount,
User user,
UserContact userContact)
{
Account = userAccount;
User = user;
Contact = userContact;
}
}
NOTE: This might be overkill but what I was going for here was
extracting the persistance criteria out from the creation of the
account itself. Hence you could use this class to unit test creation
of accounts without mocking or worrying about the db persistance layer
which is contained in the service class layer.
NewAccountand also a classUserAccount? I think you should removeNewAccountand useUserAccountinstead. you then can save almost the whole block after creating a newUserAccountand rather have it passed into the method (dependency injection). Also make yourAccountAlreadyExistsmethod accept aUserAccountas argument and not aUserName(String?) or rename it toUserNameAlreadyExists. \$\endgroup\$ifblock, then the else is a return null, you might switch that up to beif (AccountAlreadyExists(newAccount.UserName)) return null;then have the rest of your code without the nested braces \$\endgroup\$