3
\$\begingroup\$

We typically do our saves and deletes for our asp.net pages (SQL Server as back-end) in the code behind. I've seen some reference the idea of creating a stored procedure for this purpose. Is this the best approach or is what I'm doing practical enough? Any suggestions? Here's an example of the delete button function (first half is for triggers set up in SQL):

protected void DeleteClick(object sender, EventArgs e)
{
    cookie = Request.Cookies["pAuthCookie"];
    if (txtKey.Text == "")
    {
        string scriptstring = "alert('User Error - Must Select Fromn the LIST first.');";
        ScriptManager.RegisterStartupScript(Tab1, typeof(string), "alertscript", scriptstring, true);
        if (TabContainer1.ActiveTabIndex == 1)
            TabContainer1.ActiveTabIndex = 0;
        return;
    }
    else
    {
        String fprescript = txtKey.Text;
        string empno = cookie["thenum"].ToString();

        string sqlstring1 = "UPDATE prescriptions set empfk = @empfk WHERE prescriptpk = @prescriptpk";
        con.Open();
        SqlCommand cmd1 = new SqlCommand(sqlstring1, con);
        cmd1.Parameters.Add("@prescriptpk", SqlDbType.Int);
        cmd1.Parameters.Add("@empfk", SqlDbType.Int);
        cmd1.Parameters["@prescriptpk"].Value = Convert.ToInt32(fprescript);
        cmd1.Parameters["@empfk"].Value = empno.Trim();
        cmd1.ExecuteNonQuery();

        con.Close();
        string sqlstring = "DELETE FROM prescriptions WHERE prescriptpk = @prescriptpk";
        con.Open();
        SqlCommand cmd = new SqlCommand(sqlstring, con);
        cmd.Parameters.Add("@prescriptpk", SqlDbType.Int);
        cmd.Parameters["@prescriptpk"].Value = Convert.ToInt32(fprescript);
        cmd.ExecuteNonQuery();

        con.Close();
        clearoutdata();
        TabContainer1.ActiveTabIndex = 0;
        GridView1.DataBind();
    }
}
\$\endgroup\$
2
\$\begingroup\$

This is overall comments on above code

1) Encapsulate what is repeating

In your method you are opening sql connection, adding parameter and executing it one by one. Rather doing this please extract out it into one method.

 private void ExecuteNonQuery(string connectionstring, string query, IEnumerable<SqlParameter> parameters =null)
  {
            using (var connection = new SqlConnection(connectionstring))
            {
                connection.Open();
                using (var command = new SqlCommand(query, connection))
                {
                    if (parameters != null)
                    {
                        foreach (var parameter in parameters)
                        {
                            command.Parameters.Add(parameter);
                        }
                    }
                    command.ExecuteNonQuery();
                }
            }
   }

2) Always dispose the connection

In above code your opening connection and closing it, but this can lead into connection leak in case of errors. Please wrap your code into using statement or try clause like this

using (var connection = new SqlConnection())
{

}

3) Distribute your responsibility into layer

The above code is doing too much

  1. script registering
  2. Sql Connection stuff
  3. Data Populating and clearing

Please separate out your code into at least two classes 1) Fetching data class 2) UI binding stuff class

4)inline SQL vs Stored Procedure

Stored Procedure is good as your code lies at one place in db but creating Stored Procedure for one line of SQL is not a good idea. If your logic is going to be complex and big ,Use a Stored procedure other wise inline sql.

PS: I have created a gist named SQLHelper class for you to abstract out the sql connection and command things,

https://gist.github.com/paritoshmmmec/5c3855af7215ac87d787

\$\endgroup\$
1
\$\begingroup\$
  1. You can use the SqlHelper class to do Database operation. Always use specific class or library to communicate with database. DO not write same code again and again for Database communication.

  2. Use the stored procedure instead of query.

    Create procedure prescriptions_Update
    as
    @prescriptpk int,
    @empfk varchar(50)
    BEGIN
    UPDATE prescriptions set empfk = @empfk WHERE prescriptpk = @prescriptpk
    END

  3. Use below code to call the SP

    SqlParameter[] sqlParms = new SqlParameter[2];

    sqlParms[0] = new SqlParameter("@empfk", empfk);

    sqlParms[1] = new SqlParameter("@prescriptpk", prescriptpk);

    return SqlHelper.ExecuteNonQuery(ConnectionString, CommandType.StoredProcedure, "prescriptions_Update", sqlParms);
    
\$\endgroup\$
0
0
\$\begingroup\$

I think you should use stored procedure instead of sending queries from C#.

Here you are executing many queries in else block. You can combine those in stored procedure.

Stored procedures are always a safe option to prevent sql injections etc. You will send less data on network so it'll save time although it will be negligible.

As per SoC(separation of concerns principle) the database operations should be on DB side itself. If you need to change some DB operation login you don't need to change recompile and redeploy your C# code each time because you can simply change stored procedure itself.

Stored procedures are compiled and optimized by Database Server so if you are using different servers for business layer and Database your load will be distributed and it'll increase application performance also.

The other advantage is that in most cases there is already a query plan created for stored procedure and it gets reused by sql engine while in case of queries each time new query plan will be created.

So my suggestion would be to use stored procedure rather than using plain queries.

\$\endgroup\$
4
  • \$\begingroup\$ Welcome to Code Review! I edited your post into paragraphs to make it easier to read, if you think there's better places it could be broken up then please edit your post to make it more suitable for your intent. \$\endgroup\$ Oct 20 '15 at 8:41
  • 1
    \$\begingroup\$ Thanks @SuperBiasedMan. Sorry for the bad writing. I have broken it now in paragraphs. \$\endgroup\$
    – ATP
    Oct 20 '15 at 8:46
  • 2
    \$\begingroup\$ 1. Stored procedures aren't guaranteed to protect from SQL injection. 2. All queries have a plan compiled and cached (which can be reused) not just stored procedures \$\endgroup\$
    – RobH
    Oct 20 '15 at 9:13
  • \$\begingroup\$ Sorry I downvoted this response as you don't really seem to know much about databases. You make assertions that are wrong (SPs are equally vulnerable to SQL injection), and about SPs being somehow being better "for different servers". The query plan/compiliation part is true but the performance benefit is only useful in large volumes \$\endgroup\$
    – Quango
    Oct 21 '15 at 7:20

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.