current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 1 down vote favorite

I need to work with stored procedures, with the help of Entity Framework Core. This is my function:

public async Task<bool> CheckIfUserRegistered(string phoneNumber, DateTime dateOfBirth)
    {
        if (string.IsNullOrWhiteSpace(phoneNumber))
        {
            return false;
        }
        using (var cmd = _dbContext.Database.GetDbConnection().CreateCommand())
        {
            cmd.CommandText = "dbo.CheckIfUserRegistered";
            cmd.CommandType = CommandType.StoredProcedure;

            cmd.Parameters.Add(new SqlParameter("@phoneNumber", SqlDbType.NVarChar) { Value = phoneNumber });
            cmd.Parameters.Add(new SqlParameter("@dateOfBirth", SqlDbType.Date) { Value = dateOfBirth });

            cmd.Parameters.Add(new SqlParameter("@registered", SqlDbType.Bit) { Direction = ParameterDirection.Output });

            if (cmd.Connection.State != ConnectionState.Open)
            {
                cmd.Connection.Open();
            }

            await cmd.ExecuteNonQueryAsync();

            return (bool)cmd.Parameters["@registered"].Value;
        }
    }

I'm not sure, with the correctness of this function, is this the right way to work with stored procedures in EF Core? Don't I have problems with forgotten connections, memory leakings, etc?

share|improve this question
    
Minor detail, but I would rename the method to CheckIfUserRegisteredAsync, since appending Async to the end of an asynchronous method is a popular practice. You will see this pattern used a lot in the .NET Framework for example e.g. ExecuteNonQueryAsync. – Jason Evans Dec 2 at 16:10
    
@JasonEvans I've thinking about it, and decided not to do this, because absolutely all my methods are async. – Yuriy N. Dec 2 at 17:30
up vote 0 down vote

This is a rather inconvenient way of calling a stored procedure.

Let the EF take care of everything. In your example it should create and dispose the context right away. In most cases there's no need to have a field for it. I cannot verify it but something like this should work:

public async Task<bool> CheckIfUserRegistered(string phoneNumber, DateTime dateOfBirth)
{
    if (string.IsNullOrWhiteSpace(phoneNumber))
    {
        return false;
    }

    using(var ctx = new MyDbContext())
    {
        var result =
            await 
            _dbContext
            .Database
            .ExecuteSqlCommandAsync(
                "CheckIfUserRegistered @phoneNumber @dateOfBirth", 
                parameters: new[] { phoneNumber , dateOfBirth  }
            ).SingleAsync();

        return result.Value;
    } 
}
share|improve this answer
    
ExecuteSqlCommandAsync return number of rows, while I need data from procedure, what should I do? – Yuriy N. Dec 3 at 20:11

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.