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();
\$\endgroup\$
  • \$\begingroup\$ Question , would you be able to add stored procedure in your database?. \$\endgroup\$ – paritosh Oct 19 '15 at 14:50
  • \$\begingroup\$ wouldn't it get messy with a stored procedure, @paritosh? \$\endgroup\$ – Malachi Oct 19 '15 at 14:51
  • 1
    \$\begingroup\$ @Malachi : no not at all, will much better version than this. \$\endgroup\$ – paritosh Oct 19 '15 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 '15 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 '15 at 18:32
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();
\$\endgroup\$
  • 1
    \$\begingroup\$ Best practice is to have SqlConnection and SqlCommand objects within using blocks. \$\endgroup\$ – 410_Gone Oct 19 '15 at 15:24
  • 1
    \$\begingroup\$ As will as separating UI with Data Access code \$\endgroup\$ – Sergey Berezovskiy Oct 19 '15 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 '15 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.

\$\endgroup\$
  • \$\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 '15 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 '15 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 '15 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 '15 at 15:50

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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