6
\$\begingroup\$

I've created a search page on a rather large Audit table (populated by triggers on every database). There are 4 fields you can search by (and I put each textbox next to each other): User ID, Table ID, Date, Record ID. This is for programmer's use and high level managers who would know what these fields mean. Record ID is the important field that list the rows of the unique ID that were either UPDATED, INSERTED, DELETED. Like I mentioned, it's of an Audit table we use to monitor who's doing what on our database.

I display the results using ListView (since I've never used it before and it seems good for this read-only purpose). I've found myself using a ton of else-if's in the search button click. I can't help but think there's a better way. Any suggestions? The last else seems pointless. And the only thing I could put out of scope that would work was ListView1.DataBind(); hence why I repeated a lot of that info in each else if statement.

    string ID = txtSearch.Text.ToString().Trim();
    string lTABLE = ddlTable.SelectedItem.Text.ToString().Trim();
    string lRecord = txtRecordID.Text.ToString().Trim();
    string lDATE = misc_class.format_mmddyyyy(txtDate.Text).Trim();
    string constr2 = "Server=...000...; Database=Audit;uid=junk;pwd=junk;";  // eliminated real info for privacy

    if (ID == "" && lTABLE == "" && lDATE == "" && lRecord == "")  //ALL 4 blank
    {
        string query1 = "SELECT TOP 100 [pk],[databaseName],[tableName],[tablefk],[fieldname],[old],[new],[userfk],[action],[entrytime],[username] FROM AUDITACT order by entrytime desc";
        SqlDataAdapter da = new SqlDataAdapter(query1, constr2);
        DataTable table2 = new DataTable();
        da.Fill(table2);
        ListView1.DataSource = table2;
    }
    else if (lRecord != "")           // if Record ID has any value entered in it, search by it only, disregard other textboxes
    {
        string query1 = "SELECT [pk],[databaseName],[tableName],[tablefk],[fieldname],[old],[new],[userfk],[action],[entrytime],[username] FROM AUDITACT where [tablefk] = '" + lRecord + "' order by entrytime desc";
        SqlDataAdapter da = new SqlDataAdapter(query1, constr2);
        DataTable table2 = new DataTable();
        da.Fill(table2);
        ListView1.DataSource = table2;
    }

    else if (lTABLE == "" && lDATE == "" && lRecord == "" & ID != "")  //ID only
    {
        string query1 = "SELECT [pk],[databaseName],[tableName],[tablefk],[fieldname],[old],[new],[userfk],[action],[entrytime],[username] FROM AUDITACT where ([old] = '" + ID + "' or [new] = '" + ID + "') order by entrytime desc";
        SqlDataAdapter da = new SqlDataAdapter(query1, constr2);
        DataTable table2 = new DataTable();
        da.Fill(table2);
        ListView1.DataSource = table2;
    }
    else if (ID == "" && lTABLE == "" && lRecord == "" && lDATE != "")    //Date only
    {
        string query1 = "SELECT TOP 200 [pk],[databaseName],[tableName],[tablefk],[fieldname],[old],[new],[userfk],[action],[entrytime],[username] FROM AUDITACT where CONVERT(date,[entrytime]) = '" + lDATE + "' order by entrytime desc";
        SqlDataAdapter da = new SqlDataAdapter(query1, constr2);
        DataTable table2 = new DataTable();
        da.Fill(table2);
        ListView1.DataSource = table2;
    }
    else if (ID == "" && lDATE == "" & lRecord == "" && lTABLE != "")    //Table only
    {
        string query1 = "SELECT [pk],[databaseName],[tableName],[tablefk],[fieldname],[old],[new],[userfk],[action],[entrytime],[username] FROM AUDITACT where [tableName] = '" + lTABLE + "' order by entrytime desc";
        SqlDataAdapter da = new SqlDataAdapter(query1, constr2);
        DataTable table2 = new DataTable();
        da.Fill(table2);
        ListView1.DataSource = table2;
    }
    else if (ID != "" && lTABLE != "" && lDATE == "" && lRecord == "")  // User ID & Table
    {
        string query1 = "SELECT [pk],[databaseName],[tableName],[tablefk],[fieldname],[old],[new],[userfk],[action],[entrytime],[username] FROM AUDITACT where [tableName] = '" + lTABLE + "'and ([old] = '" + ID + "' or [new] = '" + ID + "') order by entrytime desc";
        SqlDataAdapter da = new SqlDataAdapter(query1, constr2);
        DataTable table2 = new DataTable();
        da.Fill(table2);
        ListView1.DataSource = table2;
    }

    else if (ID != "" && lDATE != "" && lTABLE == "" && lRecord == "")     //User ID & Date
    {
        string query1 = "SELECT [pk],[databaseName],[tableName],[tablefk],[fieldname],[old],[new],[userfk],[action],[entrytime],[username] FROM AUDITACT where CONVERT(date,[entrytime]) = '" + lDATE + "' and ([old] = '" + ID + "' or [new] = '" + ID + "') order by entrytime desc";
        SqlDataAdapter da = new SqlDataAdapter(query1, constr2);
        DataTable table2 = new DataTable();
        da.Fill(table2);
        ListView1.DataSource = table2;
    }

    else if (lTABLE != "" && lDATE != "" && ID == "")     //Table & Date
    {
        string query1 = "SELECT [pk],[databaseName],[tableName],[tablefk],[fieldname],[old],[new],[userfk],[action],[entrytime],[username] FROM AUDITACT where CONVERT(date,[entrytime]) = '" + lDATE + "' and [tableName] = '" + lTABLE + "' order by entrytime desc";
        SqlDataAdapter da = new SqlDataAdapter(query1, constr2);
        DataTable table2 = new DataTable();
        da.Fill(table2);
        ListView1.DataSource = table2;
    }
    else if (ID != "" && lTABLE != "" && lDATE != "" && lRecord == "")       // User ID, TABLE & DATE are filled out
    {
        string query1 = "SELECT [pk],[databaseName],[tableName],[tablefk],[fieldname],[old],[new],[userfk],[action],[entrytime],[username] FROM AUDITACT where CONVERT(date,[entrytime]) = '" + lDATE + "' and [tableName] = '" + lTABLE + "' and ([old] = '" + ID + "' or [new] = '" + ID + "') order by entrytime desc";
        SqlDataAdapter da = new SqlDataAdapter(query1, constr2);
        DataTable table2 = new DataTable();
        da.Fill(table2);
        ListView1.DataSource = table2;
    }
    else  // all 4 are filled out (in which case we'd choose the Record ID field to search on)
    {
        string query1 = "SELECT [pk],[databaseName],[tableName],[tablefk],[fieldname],[old],[new],[userfk],[action],[entrytime],[username] FROM AUDITACT where [tablefk] = '" + lRecord + "' order by entrytime desc";
        SqlDataAdapter da = new SqlDataAdapter(query1, constr2);
        DataTable table2 = new DataTable();
        da.Fill(table2);
        ListView1.DataSource = table2;

    }


    ListView1.DataBind();
Improve this question
\$\endgroup\$
6
  • \$\begingroup\$ Question , would you be able to add stored procedure in your database?. \$\endgroup\$
    – Paritosh
    Oct 19, 2015 at 14:50
  • \$\begingroup\$ wouldn't it get messy with a stored procedure, @paritosh? \$\endgroup\$
    – Malachi ♦
    Oct 19, 2015 at 14:51
  • 1
    \$\begingroup\$ @Malachi : no not at all, will much better version than this. \$\endgroup\$
    – Paritosh
    Oct 19, 2015 at 14:52
  • 1
    \$\begingroup\$ @paritosh, I didn't say those exact words, but I did mention a solution to that problem at the very end of my answer \$\endgroup\$
    – Malachi ♦
    Oct 19, 2015 at 15:06
  • \$\begingroup\$ @paritosh I haven't done that before, using a stored procedure as a search function. Any good links I can check out for this? \$\endgroup\$
    – pacpie
    Oct 19, 2015 at 18:32

2 Answers 2

Reset to default
7
\$\begingroup\$

First of all you should remove duplication. It's clearly that every if..else block has same code for filling DataTable and binding it to ListView. Move it to the bottom:

string query;

if (ID == "" && lTABLE == "" && lDATE == "" && lRecord == "")  //ALL 4 blank
{
    query = "SELECT TOP 100 [pk],[databaseName],[tableName],[tablefk],[fieldname],[old],[new],[userfk],[action],[entrytime],[username] FROM AUDITACT order by entrytime desc";             
}
else if (lRecord != "")
{
    query = "SELECT [pk],[databaseName],[tableName],[tablefk],[fieldname],[old],[new],[userfk],[action],[entrytime],[username] FROM AUDITACT where [tablefk] = '" + lRecord + "' order by entrytime desc";                
}
// ...      
else
{
    query = "SELECT [pk],[databaseName],[tableName],[tablefk],[fieldname],[old],[new],[userfk],[action],[entrytime],[username] FROM AUDITACT where [tablefk] = '" + lRecord + "' order by entrytime desc"; 
}

SqlDataAdapter da = new SqlDataAdapter(query, connectionString);
DataTable table = new DataTable();
da.Fill(table);
ListView1.DataSource = table;
ListView1.DataBind();

Now all your if..else blocks is just a query string generation. But here goes your biggest problem. Your application is vulnerable to SQL Injection attacks (I strongly recommend you to follow link and read article). Instead of concatenating query string you should pass values to SQL server as command parameters. And command building code can look like:

private SqlCommand CreateAuditSelectCommand(string id, string entryDate, string tableName, string record)
{
    var cmd = new SqlCommand();
    List<string> filters = new List<string>();

    if (!String.IsNullOrEmpty(id)) {
        filters.Add("([old] = @ID OR [new] = @ID}");
        cmd.Parameters.AddWithValue("@ID", id);
    }

    if (!String.IsNullOrEmpty(record)) {
        filters.Add("[tablefk] = @Record");
        cmd.Parameters.AddWithValue("@Record", record);
    }

    if (!String.IsNullOrEmpty(entryDate)) {
        filters.Add("CONVERT(date,[entrytime] = @Date");
        cmd.Parameters.AddWithValue("@Date", entryDate);
    }

    if (!String.IsNullOrEmpty(tableName)) {
        filters.Add("[tablename] = @TableName");
        cmd.Parameters.AddWithValue("@TableName", tableName);
    }

    string whereClause = filters.Any() ? "WHERE " + String.Join(" AND ", filters) : "";
    string count = filters.Any() ? "" : "TOP 100";

    var query = String.Format(
            @"SELECT {0} [pk],[databaseName],[tableName],[tablefk],[fieldname],[old],[new],[userfk],[action],[entrytime],[username]
            FROM AUDITACT {1}
            ORDER BY entrytime DESC", count, whereClause);

    cmd.CommandText = query;
    return cmd;
}

Pretty clean. You just check if parameter has value, add it command and add new filter for where clause. After that you just join all filters and you have query string ready.

Next step is separating Data Access code from UI code, and making sure that connection and command are closed after usage. Following method does not know anything about UI. Actually you can move both these methods to separate class (Repository):

private DataTable GetAuditActs(string id, string entryDate, string tableName, string record)
{
    string connectionString = ConfigurationManager.ConnectionStrings["Audit"].ConnectionString;
    using (var conn = new SqlConnection(connectionString))
    using (var cmd = CreateAuditSelectCommand(id, entryDate, tableName, record))
    {
        cmd.Connection = conn;
        conn.Open();
        SqlDataAdapter adapter = new SqlDataAdapter(cmd);
        DataTable table = new DataTable();
        adapter.Fill(table);
        return table;
    }
}

And now you have nice clean method free from any data access specific code:

string id = txtSearch.Text.ToString().Trim();
string tableName = ddlTable.SelectedItem.Text.ToString().Trim();
string record = txtRecordID.Text.ToString().Trim();
string entryDate = misc_class.format_mmddyyyy(txtDate.Text).Trim();

ListView1.DataSource = GetAuditActs(id, entryDate, tableName, record);
ListView1.DataBind();
Improve this answer
\$\endgroup\$
3
  • 1
    \$\begingroup\$ Best practice is to have SqlConnection and SqlCommand objects within using blocks. \$\endgroup\$
    – Der Kommissar
    Oct 19, 2015 at 15:24
  • 1
    \$\begingroup\$ As will as separating UI with Data Access code \$\endgroup\$
    – Sergey Berezovskiy
    Oct 19, 2015 at 15:35
  • 2
    \$\begingroup\$ This is incredible. Very detailed and thorough. I appreciate it all. I am studying it as we speak. THANK YOU! \$\endgroup\$
    – pacpie
    Oct 19, 2015 at 17:38
3
\$\begingroup\$

Your connection string should be housed in your web.config file and not inside of one of your code behind files. There is a configuration class that you can use to access this connection string.

Then you should create everything that you can outside of the if statements

like:

string query1 = new string();

then take:

    SqlDataAdapter da = new SqlDataAdapter(query1, constr2);
    DataTable table2 = new DataTable();
    da.Fill(table2);
    ListView1.DataSource = table2;

and move it after the if statement.

it will turn your code into this:

if (ID == "" && lTABLE == "" && lDATE == "" && lRecord == "")  //ALL 4 blank
{
    query1 = "SELECT TOP 100 [pk],[databaseName],[tableName],[tablefk],[fieldname],[old],[new],[userfk],[action],[entrytime],[username] FROM AUDITACT order by entrytime desc";
}
else if (lRecord != "")           // if Record ID has any value entered in it, search by it only, disregard other textboxes
{
    query1 = "SELECT [pk],[databaseName],[tableName],[tablefk],[fieldname],[old],[new],[userfk],[action],[entrytime],[username] FROM AUDITACT where [tablefk] = '" + lRecord + "' order by entrytime desc";
}
else if (lTABLE == "" && lDATE == "" && lRecord == "" & ID != "")  //ID only
{
    query1 = "SELECT [pk],[databaseName],[tableName],[tablefk],[fieldname],[old],[new],[userfk],[action],[entrytime],[username] FROM AUDITACT where ([old] = '" + ID + "' or [new] = '" + ID + "') order by entrytime desc";
}
else if (ID == "" && lTABLE == "" && lRecord == "" && lDATE != "")    //Date only
{
    query1 = "SELECT TOP 200 [pk],[databaseName],[tableName],[tablefk],[fieldname],[old],[new],[userfk],[action],[entrytime],[username] FROM AUDITACT where CONVERT(date,[entrytime]) = '" + lDATE + "' order by entrytime desc";
}
else if (ID == "" && lDATE == "" & lRecord == "" && lTABLE != "")    //Table only
{
    query1 = "SELECT [pk],[databaseName],[tableName],[tablefk],[fieldname],[old],[new],[userfk],[action],[entrytime],[username] FROM AUDITACT where [tableName] = '" + lTABLE + "' order by entrytime desc";
}
else if (ID != "" && lTABLE != "" && lDATE == "" && lRecord == "")  // User ID & Table
{
    query1 = "SELECT [pk],[databaseName],[tableName],[tablefk],[fieldname],[old],[new],[userfk],[action],[entrytime],[username] FROM AUDITACT where [tableName] = '" + lTABLE + "'and ([old] = '" + ID + "' or [new] = '" + ID + "') order by entrytime desc";
}
else if (ID != "" && lDATE != "" && lTABLE == "" && lRecord == "")     //User ID & Date
{
    query1 = "SELECT [pk],[databaseName],[tableName],[tablefk],[fieldname],[old],[new],[userfk],[action],[entrytime],[username] FROM AUDITACT where CONVERT(date,[entrytime]) = '" + lDATE + "' and ([old] = '" + ID + "' or [new] = '" + ID + "') order by entrytime desc";
}
else if (lTABLE != "" && lDATE != "" && ID == "")     //Table & Date
{
    query1 = "SELECT [pk],[databaseName],[tableName],[tablefk],[fieldname],[old],[new],[userfk],[action],[entrytime],[username] FROM AUDITACT where CONVERT(date,[entrytime]) = '" + lDATE + "' and [tableName] = '" + lTABLE + "' order by entrytime desc";
}
else if (ID != "" && lTABLE != "" && lDATE != "" && lRecord == "")       // User ID, TABLE & DATE are filled out
{
    query1 = "SELECT [pk],[databaseName],[tableName],[tablefk],[fieldname],[old],[new],[userfk],[action],[entrytime],[username] FROM AUDITACT where CONVERT(date,[entrytime]) = '" + lDATE + "' and [tableName] = '" + lTABLE + "' and ([old] = '" + ID + "' or [new] = '" + ID + "') order by entrytime desc";
}
else  // all 4 are filled out (in which case we'd choose the Record ID field to search on)
{
    query1 = "SELECT [pk],[databaseName],[tableName],[tablefk],[fieldname],[old],[new],[userfk],[action],[entrytime],[username] FROM AUDITACT where [tablefk] = '" + lRecord + "' order by entrytime desc";
}
SqlDataAdapter da = new SqlDataAdapter(query1, constr2);
DataTable table2 = new DataTable();
da.Fill(table2);
ListView1.DataSource = table2;

Then you can look at using a Using statement or two with the SqlDataAdapter and the Datatable. you should also look into parameterizing the query.

Instead of using a string for the query you should be using a SqlCommand and adding Parameters to it, then when you are done constructing the SqlCommand inside the if statement you simply add the parameters to the SqlCommand, this makes it much safer passing strings into the query.

Improve this answer
\$\endgroup\$
4
  • \$\begingroup\$ Really nice suggestions. Thank you. I just looked and realized I do have a connection string set up for this database in my WebConfig file under <connectionStrings>. In this scenario, how do I reference it? Like string constr2 = ??? \$\endgroup\$
    – pacpie
    Oct 19, 2015 at 14:51
  • \$\begingroup\$ I don't have any .net stuff installed on this computer so I had to look up the syntax, here is a good answer from SO stackoverflow.com/a/5316976/1214743 \$\endgroup\$
    – Malachi ♦
    Oct 19, 2015 at 14:54
  • 1
    \$\begingroup\$ This code is vulnerable to SQL injection. Moreover, so much of the logic is repeated, which is bad. What if a new column is added to AUDITACT: now you need to alter so many queries. \$\endgroup\$
    – BCdotWEB
    Oct 19, 2015 at 15:30
  • \$\begingroup\$ @BCdotWEB, this is not a complete rewrite of the code, it is a good start to clean up the code. I agree with you completely, but please keep in mind this is still not production code, just better than the OP. \$\endgroup\$
    – Malachi ♦
    Oct 19, 2015 at 15:50

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.