Question What is wrong on function ExecuteNonQuery to work as Best Practise ?

[ahmedsalah]

I work on c# app I need to make function make insert data or update or delete dynamically so that I do function below for insert

or update or delete but I don't know what must added or remove from function below to make function work as best practice .

public static async Task<int> ExecuteNonQuery(string sql, SqlConnection sqlconnection, DbParameter[] @params = null, CommandType cmdType = CommandType.StoredProcedure)
{
    int RecordsCount = 0;

    if (sql == "") return 0;
    
    await Task.Run(async () =>
                   {
                       using (var con = new SqlConnection(GlobalVariables.con))
                       {
                           using (var cmd = new SqlCommand() { Connection = con })
                           {
                               if (cmd.CommandTimeout < 360)
                                   cmd.CommandTimeout = 360;
                              
                               cmd.CommandText = sql;
                               cmd.CommandType = cmdType;
                               cmd.Parameters.Clear();
                              
                               if (@params != null)
                               {
                                   for (int i = 0; i < @params.Length; i++)
                                   {
                                       cmd.Parameters.Add(@params[i]);
                                   }
                               }
                              
                               try
                               {
                                   await con.OpenAsync();

                                   RecordsCount = (await cmd.ExecuteNonQueryAsync());
                               }
                               catch (Exception ex)
                               {
                                   throw new Exception(ex.Message);
                               }
                           }
                       }
                   });
    
    return RecordsCount;
}

so I do function above for make insert or update or delete

what is remaining or wrong to be best practice ?

 

[jmcilhinney]

• You're passing in a SqlConnection but ignoring it and creating a new one.
• You have two nested using blocks with no code between them so you only need one, i.e. rather than this:

using (var x = new SomeType())
{
    using (var y = new SomeOtherType())
    {
        // ...
    }
}

use this:

using (var x = new SomeType())
using (var y = new SomeOtherType())
{
    // ...
}

• You are making poor use of an object initialiser here:

using (var cmd = new SqlCommand() { Connection = con })
{
    if (cmd.CommandTimeout < 360)
        cmd.CommandTimeout = 360;

    cmd.CommandText = sql;
    cmd.CommandType = cmdType;

You use a parameterless constructor and then set two properties that could be set via a different constructor and you set one property via an object initialiser and then set three more conventionally. That if statement is also pointless, given that you just created the command object and know exactly what the CommandTimeout is. That code should be like this:

using (var cmd = new SqlCommand(sql, con) { CommandType = cmdType, CommandTimeout = 360 })
{

• What's the point in clearing the Parameters collection of a command object that you just created and thus know has no parameters?
• The Parameters collection has an AddRange method so there's no need for a loop.
• What you're doing with that try...catch is bad a number of reasons. Why are you creating your own exception with the same message instead of allowing all the information contained in the original exception to bubble up? If you have a valid reason for creating your own exception then you should be providing your own message and setting the original exception as its InnerException. If you're not going to create your own exception but you have something else useful to do, e.g. log the exception, then you should just rethrow the original exception. You can do that using throw ex; but that's bad because it truncates the stack trace to the current method. To rethrow the original exception in its full glory, just use throw;. That said, if you're not doing anything in your catch block but rethrowing an exception then you should not be catching it at all, because the end result is the same and you don't waste the time of catching an rethrowing.

 

[ahmedsalah]

can you please show me if possible function after add modifications. it is really good advices

 

[jmcilhinney]

What is stopping you implementing the suggestions yourself? Generally speaking, the way it should work is that you do what you can for yourself first and then ask us to help with the bits you can't do or that don't work.

 

[Skydiver]

Also, it the code above, it's unclear why you are doing a Task.Run() to kick off an asynchronous task, when you calling an asynchronous method anyway. Is there a bug in the SQL driver where it is actually doing work synchronously even though you call the asynchronous method?

 

[ahmedsalah]

I have more questions and I need Answer If possible
what benefit from using timeout I need to remove also.
Task.run I using before and it not give me any issue
the final method after you tell me as below so please if you have any comment please tell me

public  async Task<int> ExecuteNonQuery(string sql, SqlConnection sqlconnection, DbParameter[] @params = null, CommandType cmdType = CommandType.StoredProcedure)
    {
        int RecordsCount = 0;
      

            if (sql == "") return 0;
            await Task.Run(async () =>
            {
                using (sqlconnection = new SqlConnection(GlobalVariables.con))
                {
                    await sqlconnection.OpenAsync();
                    var insertTransaction = sqlconnection.BeginTransaction();
                    using (var cmd = new SqlCommand(sql,sqlconnection) {  Transaction = insertTransaction,CommandType=cmdType })
                    {
                  
                        cmd.CommandText = sql;
                        cmd.CommandType = cmdType;
                        cmd.Parameters.Clear();
                        if (@params != null && @params.Length > 0)
                        {
                            cmd.Parameters.AddRange(@params);

                        }
                        try
                        {
                            

                           RecordsCount = (await cmd.ExecuteNonQueryAsync());
                            insertTransaction.Commit();
                        }
                        catch (SqlException)
                        {
                            insertTransaction.Rollback();
                            throw;

                        }
                        catch (Exception)
                        {
                            throw;
                        }
                    }

                }
                

            });
        return RecordsCount;
    }

 

[jmcilhinney]

what benefit from using timeout I need to remove also.

What is the timeout for? If you don't know then maybe you should read the documentation to find out. You can look for these things for yourself first. We're to help with stuff you can't do for yourself.

Task.run I using before and it not give me any issue

That you have used something before and it didn't break is not a good reason to use it again. What is the purpose of Task.Run? Does that apply in this case? If not then using it is not a good idea.

 

[jmcilhinney]

You have tried to implement my suggestions so I appreciate that. You've done so poorly in some cases though. The first point was this:

You're passing in a SqlConnection but ignoring it and creating a new one.

You have changed your code but you're still ignoring the connection object passed in. You're still creating a new connection object here:

using (sqlconnection = new SqlConnection(GlobalVariables.con))

so you're using the parameter as a variable but ignoring the object already assigned. I think it's clear that you are just copy/pasting code from the web without taking the time to understand what it's doing. Make a decision on the connection: do you want to pass a connection object in or create a new one in the method? If you want to pass one in the stop creating a new one. If you want to create a new one then get rid of the parameter altogether. Think about what you want to do and then write code to do that.

 

[jmcilhinney]

There's also this:

You are making poor use of an object initialiser

You've changed the code to use a better constructor and make better use of the object initialiser:

using (var cmd = new SqlCommand(sql, sqlconnection) { Transaction = insertTransaction, CommandType=cmdType })
{
    cmd.CommandText = sql;
    cmd.CommandType = cmdType;

but then you still go and pointlessly set two properties! Every line of code should have a specific purpose. THINK about what you're doing and why you're doing it. Why would you set those two properties when one has already been set by the constructor and the other has already been set by the object initialiser? If you don't know what that constructor is doing or what an object initialiser is then stop writing code you don't understand and read about them.

 

[jmcilhinney]

Next we have this:

What's the point in clearing the Parameters collection of a command object that you just created and thus know has no parameters?

which you have completely ignored.
There's also this:

if you're not doing anything in your catch block but rethrowing an exception then you should not be catching it at all

and you have this:

catch (Exception)
{
    throw;
}

That's not worng per se, just pointless. The end result will be the same whether that is there or not and, if I'm not mistaken, slightly more efficient if it's not.

 

[Skydiver]

Also notice that the SqlTransaction is an IDisposable object. If you are taking time to dispose of the SqlCommand and the SqlConnection, why are you not disposing SqlTransaction?