Running queries, good design here?

Sorry for posting so many nubbin questions on ASP.net, I'm getting the hang of it slowly.

I execute queries on my pages as such (working):

<%@ Import Namespace="System.Data" %>
<%@ Import Namespace="System.Data.SqlClient" %>
<script runat="server">

    // When the registration form is submitted
    protected void regSubmit(object sender, EventArgs e)
    {
        // No erros so far
        Boolean anyError = false;
        string errorMessages = "";

        // Take all form values
        string username = txtUsername.Text;
        string password1 = txtPassword1.Text;
        string password2 = txtPassword2.Text;
        string emailAdd = txtEmail.Text;

        // Verify that username is unique
   开发者_Python百科     using (SqlConnection cn = new SqlConnection(ConfigurationManager.ConnectionStrings["MyDbConn"].ToString()))
        {
            SqlCommand cmd = new SqlCommand("SELECT COUNT(*) FROM tblUsers WHERE username = '" + username + "'", cn);
            cn.Open();
            SqlDataReader rdr = cmd.ExecuteReader(CommandBehavior.CloseConnection);
            rdr.Read();
            int result = int.Parse(rdr[0].ToString()); //read a value
        }


        statusLabel.Text = username;
    }
</script>

My question is, is this the best practise, do I have to have a 'using' block and that inner structure for every query I run, or is there a simpler way of doing it? Also, do I need to close anything off or does the garbage man take care of it?

In Classic ASP I would just have a

adoCon.execute("DELETE FROM TABLE")

or a

rsCommon.open("SELECT * FROM TABLE"), adoCon
do until rsCommon.EOF

rscommon.movenext
loop
rsCommon.close

Tallyho! Thank you for any help! Which seems a lot simpler and intuitive to me.

Tom,

You've got a few bits of bad stuff going on here (sorry to be so blunt). I'd look at the following:

• Parameterize your queries to mitigate SQL injection attacks
• Seperate your logic out into DAL and BLL layers (or at the very least, isolate it for reuse into classes where possible)

Also, Boolean anyError = false; never get's used but i'm assuming that's for later use.

On the bright side, the idea of the using block is good, so keep that up. You can find references online to help with decoupling your logic and parameterized queries.

go with the flow

jim

The "using" block is not a MUST have, but it's a good idea. It takes care of disposing of the object automatically. (Then you don't have to remember to close it, dispose of it, etc.)

The only change I'd make is I'd change the line

using (SqlConnection cn = new SqlConnection(ConfigurationManager.ConnectionStrings["MyDbConn"].ToString()))

to

using (SqlConnection cn = new SqlConnection(ConfigurationManager.ConnectionStrings["MyDbConn"].ToString(), CommandBehavior.CloseConnection))

to ensure the connection gets closed properly.

Edit - added

Now that I see the SQL injection flaw, there are other things I would change, but I'm leaving this answer as it's specific to what you were asking.

Since you say you're a newbie, I'd like to STRONGLY recommend you get familiar with the OWASP Top 10 and get VERY familiar with it. There's a lot to learn, but web app security is critical.

The using block is a best practice. However, using string concatenation to build your sql query is just plain wrong. What if someone entered the following into your username field? :

';DROP TABLE tblUsers;--

Write it like this instead:

// When the registration form is submitted
protected void regSubmit(object sender, EventArgs e)
{
    // Verify that username is unique
    using (SqlConnection cn = new SqlConnection(ConfigurationManager.ConnectionStrings["MyDbConn"].ToString()))
    using (SqlCommand cmd = new SqlCommand("SELECT COUNT(*) FROM tblUsers WHERE username = @UserName", cn))
    {
        cmd.Parameters.Add("@UserName", SqlDbType.VarChar, 50).Value = txtUserName.Text;

        cn.Open();
        int result = (int)cmd.ExecuteScalar(); 
    }

    statusLabel.Text = txtUserName.Text;
}

Also, you should really look at using the ASP.Net membership system for all of this rather than building it yourself. Membership,login, authentication, and other security code is notoriously easy to build wrong, such that you have something that you think works and don't discover until a year later you were cracked six months ago.

Try to get into the habit of using using, as it decreases the risk you'll forget to close connections and start hitting your server's connection pool limits.

If you're just getting a single value (such as a SELECT COUNT) you can use ExecuteScalar to make things a bit more compact. But as Jim says above, make sure you parameterise your queries, otherwise you're leaving yourself wide open to SQL injection attacks:

SqlCommand cmd = new SqlCommand("SELECT COUNT(*) FROM tblUsers WHERE username = @UserName", cn);
cmd.Parameters.Add("@UserName", SqlDbType.VarChar).Value = username;
cn.Open();

int result = Convert.ToInt32(new SqlCommand(sql, cn).ExecuteScalar());

The using block is not necesarry but it makes sure that when it reaches the closing bracket the object is properly disposed of.

1. Working directly with a database from your ASPX page (or code-behind) is not a good practice. Most professional projects start from a 3-tier design where the font-end calls a business layer from which the data layer is also abstracted. More on 3-tier design here.

2. Your connection will be closed at the end of your using block, which is the easiest way to handle that.