In more simplify or effecient way of coding

Question

0.00/5 (No votes)

See more:

Hi,
I would like to ask are there any other good way to revamp the following code?
Actually this is for filtering for 3 columns.

C#

protected void ddlCat_SelectedIndexChanged(object sender, EventArgs e)
       {
           string ddlCat = ((DropDownList)sender).SelectedValue;
           Session["ddlCat"] = ddlCat;
           string ddlLoc = (string)Session["ddlLoc"];
           string ddlStatus = (string)Session["ddlStatus"];

           if (ddlCat == "All")
            {
                if (ddlLoc == "All")
                {
                    if (ddlStatus == "All")
                    {
                        SqlDataSource1.SelectCommand = "SELECT manual_cat , process_loc , area , doc_no , rev , title , originator , mrc_no , effective, status  FROM dbo.TrainningManual";
                    }
                    if (ddlStatus != "All")
                    {
                        SqlDataSource1.SelectCommand = "SELECT manual_cat , process_loc , area , doc_no , rev , title , originator , mrc_no , effective, status  FROM dbo.TrainningManual WHERE status = '" + ddlStatus + "'";
                    }
                }

                if (ddlLoc != "All")
                {
                    if (ddlStatus == "All")
                    {
                        SqlDataSource1.SelectCommand = "SELECT manual_cat , process_loc , area , doc_no , rev , title , originator , mrc_no , effective, status  FROM dbo.TrainningManual WHERE process_loc = '" + ddlLoc + "'";
                    }
                    if (ddlStatus != "All")
                    {
                        SqlDataSource1.SelectCommand = "SELECT manual_cat , process_loc , area , doc_no , rev , title , originator , mrc_no , effective, status  FROM dbo.TrainningManual WHERE process_loc = '" + ddlLoc + "' and status = '" + ddlStatus + "'";
                    }
                }

            }
            else //ddlCat != "All"
            {
                if (ddlLoc != "All")
                {
                    if (ddlStatus == "All")
                    {
                        SqlDataSource1.SelectCommand = "SELECT manual_cat , process_loc , area , doc_no , rev , title , originator , mrc_no , effective, status  FROM dbo.TrainningManual WHERE manual_cat = '" + ddlCat + "'  and process_loc = '" + ddlLoc + "' ";
                    }

                    if (ddlStatus != "All")
                    {
                        SqlDataSource1.SelectCommand = "SELECT manual_cat , process_loc , area , doc_no , rev , title , originator , mrc_no , effective, status  FROM dbo.TrainningManual WHERE manual_cat = '" + ddlCat + "'  and process_loc = '" + ddlLoc + "' and status = '" + ddlStatus + "' ";
                    }
                }

                if (ddlLoc == "All")
                {
                    if (ddlStatus == "All")
                    {
                        SqlDataSource1.SelectCommand = "SELECT manual_cat , process_loc , area , doc_no , rev , title , originator , mrc_no , effective, status  FROM dbo.TrainningManual WHERE manual_cat = '" + ddlCat + "'";
                    }

                    if (ddlStatus != "All")
                    {
                        SqlDataSource1.SelectCommand = "SELECT manual_cat , process_loc , area , doc_no , rev , title , originator , mrc_no , effective, status  FROM dbo.TrainningManual WHERE manual_cat = '" + ddlCat + "' and status = '" + ddlStatus + "'";
                    }
                }
           }
           GridView1.DataBind();

}

Posted 8-May-13 20:54pm

BabyCoding

Updated 8-May-13 21:59pm

v2

Add a Solution

Comments

Zoltán Zörgő 9-May-13 4:04am

As I see, you filter only on manual_cat, process_loc and status fields. My suggestion will give you definitely the shortest code, as pushes the "code" to sql side and uses bool logic instead of decisions.

BabyCoding 9-May-13 4:30am

Thanks!how if as for now i want try to avoid put code to sql side?

Zoltán Zörgő 9-May-13 4:36am

Why? It is not business logic you delegate there - it is the filtering you do anyway. And you don't break the separation of concerns: your code is passing the filter parameters to the sql server, the sql server is doing the filtering. It is far better, than having to construct and manage several queries, and you minimize the chance of a coding mistake.

BabyCoding 9-May-13 22:55pm

Thanks! can you give sample by using my code?

Zoltán Zörgő 10-May-13 3:15am

See update

3 solutions

Solution 2

You could use switch case statements

switch (ddlCat)

case "All":
{
  switch (ddlLoc)
  case "All" 
  {
    //query+ WHERE status = '" + ddlStatus + "'";
    break;
  }
  default:
  {
    break;
  }
  break;
}

default:
{
  break;
}

Honestly though, I think If else in your case is easier to debug.

Posted 8-May-13 21:07pm

Abhinav S

Solution 3

The first thing you could do (in addition to what Zoltan suggests) is learn to use C# more efficiently.

C#

if (myBool)
   statement;
if (!myBool)
   otherStatement;

Is both clearer and more efficient as:

C#

if (myBool)
   statement;
else
   otherStatement;

You should also not get into the habit of concatenating strings at all - not just for SQL queries, but in general it is a poor idea. Look at the StringBuilder class if you need to do it often.

And if you are going to post code fragments, make sure they compile! Because that sure as heck won't!

Posted 8-May-13 21:12pm

OriginalGriff

Comments

BabyCoding 9-May-13 4:14am

you mean use if else is more efficient then multiple if?

OriginalGriff 9-May-13 4:24am

Yes. Plus, since all your conditions are string comparisons, and are mostly in pairs "strings == stringb" followed by "stringa != stringb" then you are wasting processing time by doing unnecessary string comparisons, when you know that if it passes the first test it will automatically fail the second.

Makes your code look like you didn't think about what you were doing very much! :laugh:

BabyCoding 9-May-13 5:04am

thanks!

Add a Solution

Add your solution here

Treat my content as plain text, not as HTML

Preview 0

…

Existing Members

Sign in to your account

...or Join us

Download, Vote, Comment, Publish.

Your Email
Password
Forgot your password?

Your Email
This email is in use. Do you need your password?
Optional Password

I have read and agree to the Terms of Service and Privacy Policy
Please subscribe me to the CodeProject newsletters

When answering a question please:

Read the question carefully.
Understand that English isn't everyone's first language so be lenient of bad spelling and grammar.
If a question is poorly phrased then either ask for clarification, ignore it, or edit the question and fix the problem. Insults are not welcome.
Don't tell someone to read the manual. Chances are they have and don't get it. Provide an answer or move on to the next question.

Let's work to help developers, not make them feel stupid.

This content, along with any associated source code and files, is licensed under The Code Project Open License (CPOL)

**Zoltán Zörgő** · Accepted Answer · 2013-05-08T21:03:00

First of all, don't concatenate strings when you build a query. You expose your application to SQLi.
You could use All as a wildcard (but NULL would be better):
... WHERE (field1 = @param1 OR @param1 = 'ALL') AND (field2 = @param2 OR @param2 = 'ALL')...
and so on. It won't be an optimal query - the optimizer will however deal with it. If you run such query from time to time, it won't affect performance, but you should be careful if it is ran often.

[UPDATE]
Something like this:

C#

protected void ddlCat_SelectedIndexChanged(object sender, EventArgs e)
{
   Session["ddlCat"] = ((DropDownList)sender).SelectedValue;
   
   string ddlCat = (string)Session["ddlCat"];
   string ddlLoc = (string)Session["ddlLoc"];
   string ddlStatus = (string)Session["ddlStatus"];
   
   SqlDataSource1.SelectCommand = 
      @"SELECT manual_cat , process_loc , area , doc_no , rev , title , originator , mrc_no , effective, status 
		FROM dbo.TrainningManual WHERE 
		(manual_cat = @ddlCat OR  @ddlCat='All') AND
		(process_loc = @ddlLoc OR @ddlLoc='All') AND
		(status = @ddlStatus OR @ddlStatus='All')";
		
   SqlDataSource1.SelectParameters.Add("@ddlCat", SqlDbType.VarChar, 80).Value = ddlCat;
   SqlDataSource1.SelectParameters.Add("@ddlLoc", SqlDbType.VarChar, 80).Value = ddlLoc;
   SqlDataSource1.SelectParameters.Add("@ddlStatus", SqlDbType.VarChar, 80).Value = ddlStatus;   
   
   GridView1.DataBind();
}

Be aware, that I have not tested it, you might need to refine.

You can also bind your parameters in declarative way, something like this:

XML

<asp:sqldatasource
         id="SqlDataSource1"
         runat="server"
         connectionstring="<%$ ConnectionStrings:yourconnectionstring%>"
         selectcommand="SELECT manual_cat , process_loc , area , doc_no , rev , title , originator , mrc_no , effective, status FROM dbo.TrainningManual WHERE (manual_cat = @ddlCat OR  @ddlCat='All') AND (process_loc = @ddlLoc OR @ddlLoc='All') AND (status = @ddlStatus OR @ddlStatus='All')">
         <selectparameters>
             <asp:controlparameter name="ddlCat" controlid="DropDownList1" propertyname="SelectedValue"/>
             <asp:sessionparameter name="ddlLoc" sessionfield="ddlLoc" type="String" DefaultValue="All" />
             <asp:sessionparameter name="ddlStatus" sessionfield="ddlStatus" type="String" DefaultValue="All"/>
         </selectparameters>
</asp:sqldatasource>