5
\$\begingroup\$

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?

\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – Jason Evans Dec 2 '16 at 16:10
  • 1
    \$\begingroup\$ @JasonEvans I've thinking about it, and decided not to do this, because absolutely all my methods are async. \$\endgroup\$ – Yurii N. Dec 2 '16 at 17:30
5
\$\begingroup\$

The biggest thing I would change is to return the connection to the state you found it.

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

await cmd.ExecuteNonQueryAsync();

if (!isOpen)
{
    cmd.Connection.Close();
}

Then, from there, I would create a helper method to build the SqlParameter objets for you:

private SqlParameter BuildParamter(string name, SqlDbType type, object value, ParameterDirection? direction)
{
    var parameter = new SqlParameter(name, type);

    if (value != null)
    {
        parameter.Value = value;
    }

    if (direction.HasValue)
    {
        parameter.Direction = direction.Value;
    }

    return parameter;
}

Then your parameters go from:

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 });

To:

cmd.Parameters.Add(BuildParameter("@phoneNumber", SqlDbType.NVarChar, phoneNumber));
cmd.Parameters.Add(BuildParameter("@dateOfBirth", SqlDbType.Date, dateOfBirth));

cmd.Parameters.Add(BuildParameter("@registered", SqlDbType.Bit, null, ParameterDirection.Output));

It's less verbose, but if you build a lot of SqlParameter objects it might be handy to have one method you can use for all of them. (You can optionally add a int? size parameter to the method as well.)

Then, extract a SetStoredProcedure method which would take the input SqlCommand and string, set the type and command text to "dbo." + name, then you add the parameters, then ExecuteStoredProcedure would do the connection checking and then you can return your cmd.Parameters["@registered"].Value as you are. In the end something like:

SetStoredProcedure("CheckIfUserRegistered", cmd):

cmd.Parameters.Add(BuildParameter("@phoneNumber", SqlDbType.NVarChar, phoneNumber));
cmd.Parameters.Add(BuildParameter("@dateOfBirth", SqlDbType.Date, dateOfBirth));

cmd.Parameters.Add(BuildParameter("@registered", SqlDbType.Bit, null, ParameterDirection.Output));

ExecuteStoredProcedure(cmd);

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

Optionally with your async/await pattern.

This way, if you create more stored procedures it's less work to make them operate.

\$\endgroup\$
-4
\$\begingroup\$

That's way too heavy to call a stored procedure. Here is my case. I have a SP called "DeleteBusiness" which takes 1 parameter. Entity Framework 6 will generate a method in DataModel.context.cs as follows,

public virtual int DeleteBusiness(Nullable<int> p_COMPID)
{
    var p_COMPIDParameter = p_COMPID.HasValue ?
        new ObjectParameter("p_COMPID", p_COMPID) :
        new ObjectParameter("p_COMPID", typeof(int));

    return ((IObjectContextAdapter)this).ObjectContext.ExecuteFunction("DeleteBusiness", p_COMPIDParameter);
}

Then in my code I just call the method in DbContext like this,

DbContext.DeleteBusiness(compId);
\$\endgroup\$
  • \$\begingroup\$ I'm not sure, do we have such method in EF Core? \$\endgroup\$ – Yurii N. Oct 19 '18 at 10:19
  • \$\begingroup\$ The method you are proposing will only return the number of rows affected. According to the docs on ObjectParameter it can also not deal with out parameters (aka. return values) of Stored Procedures. As such the code proposed can not be used to replace the code from the question. \$\endgroup\$ – Vogel612 Oct 19 '18 at 11:18
  • \$\begingroup\$ @YuriyN. The freamwork will generate the method. \$\endgroup\$ – J.D Oct 21 '18 at 10:57
  • \$\begingroup\$ @Vogel612 It's also possible to return a value like this, var pResult = new ObjectParameter("pResult", typeof(int)); dbContext.uspSubmitSale(data.Id, pResult); \$\endgroup\$ – J.D Oct 21 '18 at 10:58

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.