Where to check user email does not already exits?

Question

Hi people,

Im fairly sure this will have been asked before but I haven't found the answer. I have an account object that creates a user like so;

public class Account
{
    public ICollection Users { get; set; }

    public User CreateUser(string email)
    {
        User user = new User(email);
        user.Account = this;
        Users.Add(user);
    }
}

so in my service layer when creating a new user i call this method. However there is a rule that the users email MUST be unique to the account, so where does this go? To me it should go in the CreateUser method with an extra line that just checks that the email is unique to the account. However if it were to do this then ALL the users for the account would need to be loaded in and that seems like a bit of an overhead to me. It would be better to query the database for the users email - but doing that in the method would require a repository in the account object wouldn't it? Maybe the answer then is when loading the account from the repository instead of doing;

var accountRepository.Get(12);
//instead do
var accountRepository.GetWithUserLoadedOnEmail(12, "[email protected]");

Then the account object could still check the Users collection for the email and it would have been eagerly loaded in if found.

Does this work? What do you do? What would you do? Any guidance would be much appreciated.

Oh and Im using NHibernate as an ORM if that helps.

Answer 1

If you have properly specified the constraints on the users table, the add should throw an exception telling you that there is already a duplicate value. You can either catch that exception in the CreateUser method and return null or some duplicate user status code, or let it flow out and catch it later.

You don't want to test if it exists in your code and then add, because there is a slight possibility that between the test and the add, someone will come along and add the same email with would cause the exception to be thrown anyway...

public User CreateUser(string email)
{
    try
    {
       User user = new User(email);
       user.Account = this;
       user.Insert();
    catch (SqlException e)
    {
      // It would be best to check for the exception code from your db...
      return null;
    }
}

Answer 2

Given that "the rule that the users email MUST be unique to the account", then the most important thing is to specify in the database schema that the email is unique, so that the database INSERT will fail if the email is duplicate.

You probably can't prevent two users adding the same email nearly-simultaneously, so the next thing is that the code should handle (gracefully) an INSERT failure cause by the above.

After you've implemented the above, seeing whether the email is unique before you do the insert is just optional.

Answer 3

Thanks for all responses so far - would this work? If not what am I missing - am I over simplifiying something? The only thing that springs to mind is whether I should use a transaction to lock rows between loading the account and flushing the repository. That would get around the issue raised by consultutah - but at what cost?

// Service layer
try{
// will eagerly load Users collection on account 12 with all users that have matching email address
Account account = accountRepository.LoadAccountWithUsersThatMatchEmail(12, "[email protected]");  
account.CreateUser("[email protected]");
// other stuff
accountRepository.Flush();
}catch(SqlException, DuplicateEmailException){
    // Handle exception
}

// Method in Account object in domain layer
public User CreateUser(string email)
{
    foreach(User user in Users)
    {
        if(user.Email.Equals(email))
        {
            throw new DuplicateEmailException("User already exists with that email address");
        }
    }
    User user = new User(email);
    user.Account = this;
    Users.Add(user);
}

Answer 4

First off, I do not think you should use exceptions to handle "normal" business logic like checking for duplicate email addresses. This is a well document anti-pattern and is best avoided. Keep the constraint on the DB and handle any duplicate exceptions because they cannot be avoid, but try to keep them to a minimum by checking. I would not recommend locking the table.

Secondly, you've put the DDD tag on this questions, so I'll answer it in a DDD way. It looks to me like you need a domain service or factory. Once you have moved this code in a domain service or factory, you can then inject a UserRepository into it and make a call to it to see if a user already exists with that email address.

Something like this:

public class CreateUserService
{
private readonly IUserRepository userRepository;

public CreateUserService(IUserRepository userRepository)
{
    this.userRepository = userRepository;
}

public bool CreateUser(Account account, string emailAddress)
{
    // Check if there is already a user with this email address
    User userWithSameEmailAddress = userRepository.GetUserByEmailAddress(emailAddress);
    if (userWithSameEmailAddress != null)
    {
        return false;
    }

    // Create the new user, depending on you aggregates this could be a factory method on Account
    User newUser = new User(emailAddress);
    account.AddUser(newUser);
    return true;
}
}

This allows you to separate the responsiblities a little and use the domain service to coordinate things. Hope that helps!