Your design isn't really optimal; is it possible to consider storing all data in a central table linked to both Category and SubCategory?
There are several weaknesses; any string concatenation of sql leaves you open to SqlInjection attacks. Even if you are choosing values from drop down lists, for example, it is still possible for client side script to modify the values in your combo boxes, or for an attacker to simply post data to your server side event handler.
In addition, having to source data from several tables means that you may have to deal with different schemas in your results; if you expect this (i.e. some tables will have more columns than others) then you can handle it appropriately.
Your query would then become something similar to:
protected void Page_Load(object sender, EventArgs e)
{
if (IsPostBack == false)
{
string s = Request.QueryString["cat"];
string s1 = Request.QueryString["sub"];
if(String.IsNullOrEmpty(s) || String.IsNullOrEmpty(s1)) { return; } //Improve Validation and error reporting
using(SqlConnection conn = new SqlConnection("Data Source=ANURAG-PC;Initial Catalog=dbPortal;Persist Security Info=True;User ID=sa;Password=anurag"))
{
using(SqlCommand command = new SqlCommand(conn))
{
command.CommandType = CommandType.Text;
command.CommandText = "SELECT * FROM Table WHERE Category = @Category AND SubCategory = @SubCategory";
command.Parameters.Add(new SqlParameter() { Type = SqlDbType.String, Name = "@Category", Value = s });
command.Parameters.Add(new SqlParameter() { Type = SqlDbType.String, Name = "@SubCategory", Value = s1 });
conn.Open();
using(SqlDataReader reader = command.ExecuteReader())
{
DataTable data = new DataTable("MyData");
data.Load(reader);
DataGrid1.DataSource = data;
DataGrid1.DataBind();
}
}
}
}
}
If you are stuck with your original model, then you may want to whitelist the table names so you can stick with parameterised queries:
protected void Page_Load(object sender, EventArgs e)
{
if (IsPostBack == false)
{
string s = Request.QueryString["cat"];
string s1 = Request.QueryString["sub"];
if(String.IsNullOrEmpty(s) || String.IsNullOrEmpty(s1)) { return; } //Improve Validation and error reporting
using(SqlConnection conn = new SqlConnection("Data Source=ANURAG-PC;Initial Catalog=dbPortal;Persist Security Info=True;User ID=sa;Password=anurag"))
{
using(SqlCommand command = new SqlCommand(conn))
{
command.CommandType = CommandType.Text;
switch(s)
{
case "Architect":
command.CommandText = "SELECT * FROM Architect WHERE SubCategory = @SubCategory";
break;
case "SomethingElse":
command.CommandText = "SELECT * FROM SomethingElse WHERE SubCategory = @SubCategory";
break;
default:
return; //Again, improve error handling
}
command.Parameters.Add(new SqlParameter() { Type = SqlDbType.String, Name = "@SubCategory", Value = s1 });
conn.Open();
using(SqlDataReader reader = command.ExecuteReader())
{
DataTable data = new DataTable("MyData");
data.Load(reader);
DataGrid1.DataSource = data;
DataGrid1.DataBind();
}
}
}
}
}
One comment I would make though, is that even if you implement either of the examples above, you still have a big problem; your data access code, business logic, and presentation code are all now munged into the code behind for this page. You will have to repeat this everywhere you need it leading to plenty of duplication, which is especially a problem when you need to fix bugs.
Instead, you might consider creating classes or using an ORM to handle all of this work for you, so you instead request a list of Architect objects, or a list of SomethingElse from a class or component, thus leaving the aspx to deal with the presentation. There is also a discussion here about why you might not want to use an ORM.
If you follow this route, your code might then become something like:
protected void Page_Load(object sender, EventArgs e)
{
if (IsPostBack == false)
{
string s = Request.QueryString["cat"];
string s1 = Request.QueryString["sub"];
//Still do validation on s and s1
ObjectFactory of = new ObjjectFactory();
DataGrid1.DataSource = ObjectFactory.GetObjects(s, s1);
DataGrid1.DataBind();
}
}
Effectively, it is now someone else's job to worry about how to get the objects, and to collect them, vastly reducing the code you have in your code behind. Plus you can easily reuse that across a wide variety of interfaces!
