I need an advice for my code. What I want to do is insert a row into a table using Entity Framework Core in ASP.NET Core.

Before inserting new data, I want to check if email and phone number is already used or not.

I want to return specifically, example if return = x, email used. If return = y, phone used.

Here's my code

public int Insert(Employee employee)
{
    var checkEmail = context.Employees.Single(e => e.Email == employee.Email);

    if (checkEmail != null)
    {
        var checkPhone = context.Employees.Single(e => e.Phone == employee.Phone);

        if (checkPhone != null)
        {
            context.Employees.Add(employee);
            context.SaveChanges();
            return 1;
        }

        return 2;
    }

    return 3;
}

I'm not sure with my code, is there any advice for the best practice in my case?

CodePudding user response：

I just don't like these "magic numbers" that indicate the result of your checks.... how are you or how is anyone else going to know what 1 or 2 means, 6 months down the road from now??

I would suggest to either at least create a constants class that make it's more obvious what these numbers mean:

public class CheckConstants
{
    public const int Successful = 1;
    public const int PhoneExists = 2;
    public const int EmailExists = 3;
}

and then use these constants in your code:

public int Insert(Employee employee)
{
    var checkEmail = context.Employees.Single(e => e.Email == employee.Email);

    if (checkEmail != null)
    {
        var checkPhone = context.Employees.Single(e => e.Phone == employee.Phone);

        if (checkPhone != null)
        {
            context.Employees.Add(employee);
            context.SaveChanges();

            return CheckConstants.Successful;
        }

        return CheckConstants.PhoneExists;
    }

    return CheckConstants.EmailExists;
}

and also in any code that calls this method and need to know about the return status code.

Alternatively, you could also change this to an enum (instead of an int):

public enum CheckConstants
{
    Successful, PhoneExists, EmailExists
}

and then just return this enum - instead of an int - from your method:

public CheckConstants Insert(Employee employee)
{
    var checkEmail = context.Employees.Single(e => e.Email == employee.Email);

    if (checkEmail != null)
    {
        var checkPhone = context.Employees.Single(e => e.Phone == employee.Phone);

        if (checkPhone != null)
        {
            context.Employees.Add(employee);
            context.SaveChanges();

            return CheckConstants.Successful;
        }

        return CheckConstants.PhoneExists;
    }

    return CheckConstants.EmailExists;
}

CodePudding user response：

1. merge two database check to one Query

2. use SingleOrDefault instance of Single

   public int Insert(Employee employee)
   {
       var checkEmail = context.Employees.Select (e=>new {e.Email , e.Phone }).SingleOrDefault(e => e.Email == employee.Email || e.Phone == employee.Phone);
   
       if (checkEmail == null)
       {
           context.Employees.Add(employee);
           context.SaveChanges();
           return 1;
       }
       else if (checkEmail.Email == employee.Email)
           return 3;
       else
           return 2;
   }