CODEWITHSUNDEEP
c#asp.netsql-server
HasanG
HasanG

Reputation: 13161

Right code to retrieve data from sql server database

I have some problems in database connection and wonder if I have something wrong in my code. Please review. This question is related: Switch between databases, use two databases simultaneously question.

cs="Data Source=mywebsite.com;Initial Catalog=database;User Id=root;Password=toor;Connect Timeout=10;Pooling='true';"

using (SqlConnection cnn = new SqlConnection(WebConfigurationManager.ConnectionStrings["cs"].ConnectionString))
{
    using (SqlCommand cmmnd = new SqlCommand("", cnn))
    {
        try
        {
            cnn.Open();

            #region Header & Description
            cmmnd.Parameters.Add("@CatID", SqlDbType.Int).Value = catId;
            cmmnd.CommandText = "SELECT UpperID, Title, Description FROM Categories WHERE CatID=@CatID;";

            string mainCat = String.Empty, rootCat = String.Empty;

            using (SqlDataReader rdr = cmmnd.ExecuteReader())
            {
                if (rdr.Read())
                {
                    mainCat = rdr["Title"].ToString();
                    upperId = Convert.ToInt32(rdr["UpperID"]);
                    description = rdr["Title"];
                }
                else { Response.Redirect("/", false); }
            }

            if (upperId > 0) //If upper category exists add its name
            {
                cmmnd.Parameters["@CatID"].Value = upperId;
                cmmnd.CommandText = "SELECT Title FROM Categories WHERE CatID=@CatID;";
                using (SqlDataReader rdr = cmmnd.ExecuteReader())
                {
                    if (rdr.Read())
                    {
                        rootCat = "<a href='x.aspx'>" + rdr["Title"] + "</a> &raquo; ";
                    }
                }
            }
            #endregion

            #region Sub-Categories
            if (upperId == 0) //show only at root categories
            {
                cmmnd.Parameters["@CatID"].Value = catId;
                cmmnd.CommandText = "SELECT Count(CatID) FROM Categories WHERE UpperID=@CatID;";

                if (Convert.ToInt32(cmmnd.ExecuteScalar()) > 0)
                {
                    cmmnd.CommandText = "SELECT CatID, Title FROM Categories WHERE UpperID=@CatID ORDER BY Title;";

                    using (SqlDataReader rdr = cmmnd.ExecuteReader())
                    {
                        while (rdr.Read())
                        {
                            subcat.InnerHtml += "<a href='x.aspx'>" + rdr["Title"].ToString().ToLower() + "</a>\n";
                            description += rdr["Title"] + ", ";
                        }
                    }
                }
            }
            #endregion
        }
        catch (Exception ex) { HasanG.LogException(ex, Request.RawUrl, HttpContext.Current); Response.Redirect("/", false); }
        finally { cnn.Close(); }
    }
}

The random errors I'm receiving are:

Upvotes: 1

Views: 11971

Answers (4)

Chris Gessler
Chris Gessler

Reputation: 23113

If you're connecting remotely to a database provider, you need to look at several possibilities like your own network configuration, firewall setup, etc.

Use a packet sniffer to figure out if lost packets are the issue.

Connection pooling is setup on your local machine, the server making the connections. If the database provider only allows for 5 connections and your connection pool is setup for 50 connections, well... you can do the math. It looks like you're closing the connections properly, so no issues there.

True... one error on "description = rdr["Title"];", should that be "description = rdr["Description"].ToString()"?

No need to put a using statement around the SqlCommand object and since you're using ad-hoc queries, just use string.Format("sql test {0}", param). This way, you can reuse the SqlCommand object without having to clear the parameters.

The biggest issue I see here is that you've mixed the presentation layer with the business layer with the datasource layer. Dump the try...catch and allow the business layer to handle logging stuff. Return an object to the presentation layer and allow it to perform the redirects. Keep the datasource layer very simple... get the data and return an entity. The business layer can handle any business logic on the entity itself.

SQL Server not found could be your fault or the providers... if the provider is at fault often, change providers.

Upvotes: 1

ShahidAzim
ShahidAzim

Reputation: 1494

There are couple of inconsistencies which need to fixed:

  1. description = rdr["Title"]; no proper casting defined.
  2. Same command object is used for each sql statement and even you are not clearing parameters, it would be ideal if a separate command should be used for each sql statement.
  3. Too many redirections as well, it is best to handle redirection at the end of method.
  4. Check the database server health as well, it looks like database server is not responsive enough.

Hope it will help.

Upvotes: 1

David Weiser
David Weiser

Reputation: 5195

Are you sure that the DB is configured to grant remote access using TCP?

Upvotes: 0

ChrisLively
ChrisLively

Reputation: 88044

There's no real issues here.

You don't need the extraneous finally { cnn.close(); } as the using clause will take care of that for you. However changing it will have exactly zero impact.

Another thing is that I would put the try .. catch outside of the using clause with a redirect. But, again, I don't think that would affect the dispose from being called.

It's interesting that you would get connection pool errors (timeout expired) if you are always properly disposing of your connections, as you've shown.

Which leaves us with only one real solution: switch hosting providers. They have either overloaded their DB server to the point of unusability or some hardware element in their network setup (nic, switch, router, etc) is bad and dropping packets.

Upvotes: 3

Related Questions