I am joining 4 tables in one SQL statement that reads data into objects and fill a gridview as well.
My question: Is this a good practice ? does it have any side effects like performance when reading from the database? if so please provide me with some tips in improving it.
protected void OrdersGridView_SelectedIndexChanged(object sender, EventArgs e)
string OID = OrdersGridView.SelectedRow.Cells.Text;
OrderIDlbl.Text = "Order# " + OID;
using (SqlConnection con = new SqlConnection(cData.CS))
string sql = "select o.*, c.*, oi.*, p.* from Orders as o INNER JOIN Customers as c ON o.CustID = c.CustomerID INNER JOIN OrderItems as oi ON o.OrderID = oi.InvoiceID INNER JOIN Products as p ON p.PartNumber = oi.PartNumb where OrderID ='" + OID + "'";
SqlCommand myCommand = new SqlCommand(sql, con);
myCommand.CommandTimeout = 15;
myCommand.CommandType = CommandType.Text;
using (SqlDataReader myReader = myCommand.ExecuteReader())
passid.Text = (myReader["CustID"].ToString());
TermsDropdown.Value = (myReader["PaymentTerms"].ToString());
PaymentDate.Value = ((DateTime)myReader["PaymentDate"]).ToString("MMMM dd, yyyy");
OrderDate.Value = ((DateTime)myReader["OrderDate"]).ToString("MMMM dd, yyyy");
SalesRep.Value = (myReader["SalesRep"].ToString());
comenttxtbox.Value = (myReader["Comments"].ToString());
Discountlbl.Text = "Discount: " + (myReader["Discount"].ToString() + " AED");
Totallbl.Text = "Total: " + (myReader["Total"].ToString() + " AED");
Statuslbl.Text = (myReader["OrderStatus"].ToString());
SelectCustomertxtbox.Value = (myReader["Company"].ToString());
Name.Text = "Name: " + (myReader["FName"].ToString()) + " " + (myReader["LName"].ToString());
Phone.Text = "Phone: " + (myReader["Phone"].ToString());
Mail.Text = "Mail: " + (myReader["Personal_Email"].ToString());
DataTable dt = new DataTable();
using (SqlDataAdapter da = new SqlDataAdapter(myCommand))
OrderItemsGridview.DataSource = dt;
OrderItemsGridview.EmptyDataText = "No Items";
I wouldn't necessarily say it is a bad practice to do joins, however you should definitely get rid of the alias.* pattern that is emerging in your queries. Not explicitly selecting which columns you need is a bad practice in code.
As for the join, often it is the only way if you do not have control over the database design. However, you could greatly simplify your in-code SQL query by creating a view which does those joins for you. I would, however, question whether or not an INNER JOIN is what you actually want. Keep in mind an inner join only shows results that contain a match on both sides of the join.
In your particular case, I recommend separating the queries for the order information and the order items themselves. You're adding redundant information by including the order info on every order item. Also, whenever your query requires outside input, make sure to use a parameterized query:
myCommand.CommandText = "select o.Date, o.Title, o.Id from Orders where Id = @Id"; myCommand.Parameters.Add("@Id", SqlDbType.Int); myCommand.Parameters["@Id"].Value = 3;
Don't forget to get rid of the alias.* stuff in favor of explicitly selecting your columns.