locked
How is wrong on function ExecuteNonQuery to work as Best Practise ? RRS feed

  • Question

  • 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 ?


    Saturday, June 13, 2020 12:50 AM

Answers

  • Hello,

    Sorry, I don't see contributing to the current code as helpful as it goes against what I would see as best practices and have never written a method as shown. Any code below has been taken from either work or code samples I have on hand rather than write new code samples.

    In regards to time out, generally speaking I've only used them when working with bulk operations not a typical query.

    Here is an example for using a transaction working on two tables. Transactions do nothing for working a single action such as inserting one record as when the insert fails there is nothing to do.

    public void InsertExcelRecords(string pBin, List<ItemW2> recordList)
    {
        mHasException = false;
    
        var insertStatement =
            "INSERT INTO dbo.tblW2 " +
            "(AuditNumber,[Year],SSN,LastName,FirstName,MiddleInitial,Amount,ModifiedDate,ModifiedBy) " +
            "VALUES (@AuditNumber,@Year,@SSN,@LastName,@FirstName,@MiddleInitial,@Amount,@CreatedDate,@CreatedBy);";
    
    
        using (var cn = new SqlConnection() { ConnectionString = ConnectionStringLocalDb })
        {
    
            cn.Open();
    
            var insertTransaction = cn.BeginTransaction();
    
            using (var cmd = new SqlCommand() { Connection = cn, Transaction = insertTransaction })
            {
    
                cmd.CommandText = insertStatement;
    
                cmd.Parameters.Add(new SqlParameter("@AuditNumber", SqlDbType.Int));
                cmd.Parameters.Add(new SqlParameter("@Year", SqlDbType.Int));
                cmd.Parameters.Add(new SqlParameter("@SSN", SqlDbType.VarChar));
                cmd.Parameters.Add(new SqlParameter("@LastName", SqlDbType.VarChar));
                cmd.Parameters.Add(new SqlParameter("@FirstName", SqlDbType.VarChar));
                cmd.Parameters.Add(new SqlParameter("@MiddleInitial", SqlDbType.VarChar));
                cmd.Parameters.Add(new SqlParameter("@Amount", SqlDbType.Decimal));
                cmd.Parameters.Add(new SqlParameter("@CreatedDate", SqlDbType.DateTime));
                cmd.Parameters.Add(new SqlParameter("@CreatedBy", SqlDbType.NVarChar));
    
                try
                {
                    foreach (var row in recordList)
                    {
                        cmd.Parameters["@AuditNumber"].Value = row.AuditNumber;
                        cmd.Parameters["@Year"].Value = row.Year;
                        cmd.Parameters["@Amount"].Value = row.Amount;
                        cmd.Parameters["@SSN"].Value = string.IsNullOrWhiteSpace(row.SSN) ? "" : row.SSN;
                        cmd.Parameters["@LastName"].Value = string.IsNullOrWhiteSpace(row.LastName) ? "" : row.LastName.ToUpper();
                        cmd.Parameters["@FirstName"].Value = string.IsNullOrWhiteSpace(row.FirstName) ? "" : row.FirstName.ToUpper();
                        cmd.Parameters["@MiddleInitial"].Value = string.IsNullOrWhiteSpace(row.MiddleName) ? "" : row.MiddleName.ToUpper();
                        cmd.Parameters["@CreatedDate"].Value = DateTime.Now;
                        cmd.Parameters["@CreatedBy"].Value = Environment.UserName;
    
                        cmd.ExecuteNonQuery();
    
                    }
    
                    insertTransaction.Commit();
    
                }
                catch (SqlException sex)
                {
                    insertTransaction.Rollback();
    
                    mHasException = true;
                    mLastException = sex;
                }
                catch (Exception ex)
                {
                    mHasException = true;
                    mLastException = ex;
                }
            }
        }
    
    }
     

    A do nothing other than demonstrate using a cancellation action.

    In a form

    private CancellationTokenSource _cancellationTokenSource = new CancellationTokenSource();

    Then a run and cancel button along with an event (ignore the event it just happens to be there). Take note on -> _cancellationTokenSource.IsCancellationRequested which would be needed if the action in the Click event were to be done multiple times, we need to new up the object above.

    private async void RunButton_Click(object sender, EventArgs e)
    {
        RunButton.Enabled = false;
    
        if (_cancellationTokenSource.IsCancellationRequested)
        {
            _cancellationTokenSource.Dispose();
            _cancellationTokenSource = new CancellationTokenSource();
        }
    
        var ops = new Operations();
        ops.OnMonitor += Ops_OnMonitor;
    
        try
        {
            var resultValue = await ops.Run(_totalIterations, _cancellationTokenSource.Token);
    
        }
        catch (OperationCanceledException oce)
        {
            MessageBox.Show("Operation cancelled");
        }
        catch (Exception ex)
        {
            MessageBox.Show(ex.Message);
        }
        finally
        {
            RunButton.Enabled = true;
        }
    }
    
    private void Ops_OnMonitor(MonitorArgs args)
    {
        progressBar1.Value = args.Value;
        if (args.Value == _totalIterations)
        {
            MessageBox.Show(args.Text);
        }
    }
    
    private void CancelButton_Click(object sender, EventArgs e)
    {
        _cancellationTokenSource.Cancel();
        RunButton.Enabled = true;
    }
    
    

    The code which does nothing which is to show how to watch for a cancellation.

    using System.Threading.Tasks;
    using static CancellationToken.DelegatesModule;
    
    namespace CancellationToken
    {
        public class Operations
        {
            public event MonitorHandler OnMonitor;
            /// <summary>
            /// Do nothing method to show how to cancel a Task via
            /// CancellationTokenSource
            /// </summary>
            /// <param name="value">int value greater than 0</param>
            /// <param name="token">Initialized cancellation token</param>
            /// <returns></returns>
            public async Task<int> Run(int value, System.Threading.CancellationToken token)
            {
                var currentIndex = 0;
    
                while (currentIndex <= value -1)
                {
    
                    OnMonitor?.Invoke(new MonitorArgs("Working", currentIndex));
    
                    currentIndex += 1;
    
                    await Task.Delay(1, token);
                    
                    if (token.IsCancellationRequested)
                    {
                        token.ThrowIfCancellationRequested();
                    }
                }
    
                OnMonitor?.Invoke(new MonitorArgs("Done", currentIndex));
                return currentIndex;
            }
        }
    }
    In regards to possible failure to open a database connection, there is more to it than simple using OpenAsync and a Try/Catch, also consider what might happen if the user while waiting for a connection attempts an operation which also is dependent on the current in progress operation, that and similar things need consideration. Best way to work through this is to alter your connection string so it will not connect e.g. Data Source = .\SQLEXPRESS_BAD


    Please remember to mark the replies as answers if they help and unmarked them if they provide no help, this will help others who are looking for solutions to the same or similar problem. Contact via my Twitter (Karen Payne) or Facebook (Karen Payne) via my MSDN profile but will not answer coding question on either.

    NuGet BaseConnectionLibrary for database connections.

    StackOverFlow
    profile for Karen Payne on Stack Exchange

    Saturday, June 13, 2020 10:05 AM

All replies

  • Best practices is to have a method for each type of operation e.g. an Add new, edit/update, remove, read etc. This makes maintaining your code easier and easier to debug. Other than that you might consider using a CancellationToken at least for the connection in the event there is trouble connecting which entails adding an additional catch and try/catch by the caller checking for this else the exception is propagated to the caller..



    Please remember to mark the replies as answers if they help and unmarked them if they provide no help, this will help others who are looking for solutions to the same or similar problem. Contact via my Twitter (Karen Payne) or Facebook (Karen Payne) via my MSDN profile but will not answer coding question on either.

    NuGet BaseConnectionLibrary for database connections.

    StackOverFlow
    profile for Karen Payne on Stack Exchange

    Saturday, June 13, 2020 2:18 AM
  • I have some questions if possible answer me 

    1- Are using timeout is necessary for insert and update and delete or nothing necessary

    2- also can you help me adding transaction to function above and show me function after add transaction .

    3-can you show me when add cancellation token .

    4- are connection must open on try above as con.openaync or no need .

    5-suppose I need detect command type automatically without add commandtype so what I do or change .

    6- trouble connecting which entails adding an additional catch and try/catch by the caller checking for this  so how to check this by code

    if possible can you show modification after add modification above please 



    Saturday, June 13, 2020 7:06 AM
  • Hello,

    Sorry, I don't see contributing to the current code as helpful as it goes against what I would see as best practices and have never written a method as shown. Any code below has been taken from either work or code samples I have on hand rather than write new code samples.

    In regards to time out, generally speaking I've only used them when working with bulk operations not a typical query.

    Here is an example for using a transaction working on two tables. Transactions do nothing for working a single action such as inserting one record as when the insert fails there is nothing to do.

    public void InsertExcelRecords(string pBin, List<ItemW2> recordList)
    {
        mHasException = false;
    
        var insertStatement =
            "INSERT INTO dbo.tblW2 " +
            "(AuditNumber,[Year],SSN,LastName,FirstName,MiddleInitial,Amount,ModifiedDate,ModifiedBy) " +
            "VALUES (@AuditNumber,@Year,@SSN,@LastName,@FirstName,@MiddleInitial,@Amount,@CreatedDate,@CreatedBy);";
    
    
        using (var cn = new SqlConnection() { ConnectionString = ConnectionStringLocalDb })
        {
    
            cn.Open();
    
            var insertTransaction = cn.BeginTransaction();
    
            using (var cmd = new SqlCommand() { Connection = cn, Transaction = insertTransaction })
            {
    
                cmd.CommandText = insertStatement;
    
                cmd.Parameters.Add(new SqlParameter("@AuditNumber", SqlDbType.Int));
                cmd.Parameters.Add(new SqlParameter("@Year", SqlDbType.Int));
                cmd.Parameters.Add(new SqlParameter("@SSN", SqlDbType.VarChar));
                cmd.Parameters.Add(new SqlParameter("@LastName", SqlDbType.VarChar));
                cmd.Parameters.Add(new SqlParameter("@FirstName", SqlDbType.VarChar));
                cmd.Parameters.Add(new SqlParameter("@MiddleInitial", SqlDbType.VarChar));
                cmd.Parameters.Add(new SqlParameter("@Amount", SqlDbType.Decimal));
                cmd.Parameters.Add(new SqlParameter("@CreatedDate", SqlDbType.DateTime));
                cmd.Parameters.Add(new SqlParameter("@CreatedBy", SqlDbType.NVarChar));
    
                try
                {
                    foreach (var row in recordList)
                    {
                        cmd.Parameters["@AuditNumber"].Value = row.AuditNumber;
                        cmd.Parameters["@Year"].Value = row.Year;
                        cmd.Parameters["@Amount"].Value = row.Amount;
                        cmd.Parameters["@SSN"].Value = string.IsNullOrWhiteSpace(row.SSN) ? "" : row.SSN;
                        cmd.Parameters["@LastName"].Value = string.IsNullOrWhiteSpace(row.LastName) ? "" : row.LastName.ToUpper();
                        cmd.Parameters["@FirstName"].Value = string.IsNullOrWhiteSpace(row.FirstName) ? "" : row.FirstName.ToUpper();
                        cmd.Parameters["@MiddleInitial"].Value = string.IsNullOrWhiteSpace(row.MiddleName) ? "" : row.MiddleName.ToUpper();
                        cmd.Parameters["@CreatedDate"].Value = DateTime.Now;
                        cmd.Parameters["@CreatedBy"].Value = Environment.UserName;
    
                        cmd.ExecuteNonQuery();
    
                    }
    
                    insertTransaction.Commit();
    
                }
                catch (SqlException sex)
                {
                    insertTransaction.Rollback();
    
                    mHasException = true;
                    mLastException = sex;
                }
                catch (Exception ex)
                {
                    mHasException = true;
                    mLastException = ex;
                }
            }
        }
    
    }
     

    A do nothing other than demonstrate using a cancellation action.

    In a form

    private CancellationTokenSource _cancellationTokenSource = new CancellationTokenSource();

    Then a run and cancel button along with an event (ignore the event it just happens to be there). Take note on -> _cancellationTokenSource.IsCancellationRequested which would be needed if the action in the Click event were to be done multiple times, we need to new up the object above.

    private async void RunButton_Click(object sender, EventArgs e)
    {
        RunButton.Enabled = false;
    
        if (_cancellationTokenSource.IsCancellationRequested)
        {
            _cancellationTokenSource.Dispose();
            _cancellationTokenSource = new CancellationTokenSource();
        }
    
        var ops = new Operations();
        ops.OnMonitor += Ops_OnMonitor;
    
        try
        {
            var resultValue = await ops.Run(_totalIterations, _cancellationTokenSource.Token);
    
        }
        catch (OperationCanceledException oce)
        {
            MessageBox.Show("Operation cancelled");
        }
        catch (Exception ex)
        {
            MessageBox.Show(ex.Message);
        }
        finally
        {
            RunButton.Enabled = true;
        }
    }
    
    private void Ops_OnMonitor(MonitorArgs args)
    {
        progressBar1.Value = args.Value;
        if (args.Value == _totalIterations)
        {
            MessageBox.Show(args.Text);
        }
    }
    
    private void CancelButton_Click(object sender, EventArgs e)
    {
        _cancellationTokenSource.Cancel();
        RunButton.Enabled = true;
    }
    
    

    The code which does nothing which is to show how to watch for a cancellation.

    using System.Threading.Tasks;
    using static CancellationToken.DelegatesModule;
    
    namespace CancellationToken
    {
        public class Operations
        {
            public event MonitorHandler OnMonitor;
            /// <summary>
            /// Do nothing method to show how to cancel a Task via
            /// CancellationTokenSource
            /// </summary>
            /// <param name="value">int value greater than 0</param>
            /// <param name="token">Initialized cancellation token</param>
            /// <returns></returns>
            public async Task<int> Run(int value, System.Threading.CancellationToken token)
            {
                var currentIndex = 0;
    
                while (currentIndex <= value -1)
                {
    
                    OnMonitor?.Invoke(new MonitorArgs("Working", currentIndex));
    
                    currentIndex += 1;
    
                    await Task.Delay(1, token);
                    
                    if (token.IsCancellationRequested)
                    {
                        token.ThrowIfCancellationRequested();
                    }
                }
    
                OnMonitor?.Invoke(new MonitorArgs("Done", currentIndex));
                return currentIndex;
            }
        }
    }
    In regards to possible failure to open a database connection, there is more to it than simple using OpenAsync and a Try/Catch, also consider what might happen if the user while waiting for a connection attempts an operation which also is dependent on the current in progress operation, that and similar things need consideration. Best way to work through this is to alter your connection string so it will not connect e.g. Data Source = .\SQLEXPRESS_BAD


    Please remember to mark the replies as answers if they help and unmarked them if they provide no help, this will help others who are looking for solutions to the same or similar problem. Contact via my Twitter (Karen Payne) or Facebook (Karen Payne) via my MSDN profile but will not answer coding question on either.

    NuGet BaseConnectionLibrary for database connections.

    StackOverFlow
    profile for Karen Payne on Stack Exchange

    Saturday, June 13, 2020 10:05 AM