2
\$\begingroup\$

I feel like this maybe to much for one method... This method lives inside of a service class which is called by an interface. It dumps data into three different tables within a database.

public UserAccount CreateAccount(NewAccount newAccount)
{
  if (!AccountAlreadyExists(newAccount.UserName))
  {
    string salt = StringCipher.GenerateCypher();
    UserAccount account = new UserAccount()
    {
      UserId = newAccount.UserName,
      PasswordHash = CreatePasswordHash(newAccount.Password),
      Lock = true,
      Created = DateTime.UtcNow
    };
    User user = new User()
    {
      UserId = newAccount.UserName
    };
    UserContact contact = new UserContact() 
    {
      ContactTypeId = ContactType.EmailPrimary,
      Contact = StringCipher.Encrypt(newAccount.UserEmail, salt),
      UserId = account.UserId,
      Active = true,
      Created = DateTime.UtcNow
    };
    db.Entry(account).State = EntityState.Added;
    db.Entry(contact).State = EntityState.Added;
    db.Entry(user).State = EntityState.Added;
    db.SaveChanges();
    return account;
  }
  return null;
}

any suggestions on how to slim this down or streamline?

\$\endgroup\$
3
  • 1
    \$\begingroup\$ Why is there a class NewAccount and also a class UserAccount? I think you should remove NewAccount and use UserAccount instead. you then can save almost the whole block after creating a new UserAccount and rather have it passed into the method (dependency injection). Also make your AccountAlreadyExists method accept a UserAccount as argument and not a UserName(String?) or rename it to UserNameAlreadyExists. \$\endgroup\$ – valenterry Jul 6 '14 at 0:34
  • \$\begingroup\$ Already scratched the NewAccount class and am just using the view model that calls the interface for all this to begin with. \$\endgroup\$ – aaronmallen Jul 6 '14 at 0:40
  • \$\begingroup\$ It might be a personal preference, but you have a bit if block, then the else is a return null, you might switch that up to be if (AccountAlreadyExists(newAccount.UserName)) return null; then have the rest of your code without the nested braces \$\endgroup\$ – Prescott Jul 6 '14 at 1:16
1
\$\begingroup\$

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.

\$\endgroup\$
1

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.