Parameterized search query in C#

Question

In the UI I have a single search text box into which users can type a query. The query will split the string and compare each word to each configured column in the database and return the matches.

The following code works, but I don't know if there is a better way that I'm missing.

/// <summary>
/// Returns a list of users, either active, inactive or all
/// and can perform a search for specific users
/// </summary>
/// <param name="status">1 = active 2 = inactive 3 = all</param>
/// <param name="query">string to use for search or blank = no search</param>
/// <returns>List of Users</returns>
public static List<UM_UserData> GetUsers(int status, string query)
{
 List<UM_UserData> data = new List<UM_UserData>();
 using (MySqlConnection con = new MySqlConnection(Cfg.connectionString))
 {
 con.Open();
 string sql = GetUsersQuery(status, query);
 using (MySqlCommand cmd = new MySqlCommand(sql, con))
 {
 if (status != 3)
 {
 cmd.Parameters.AddWithValue("@status", status);
 }
 if(query != "")
 {
 string[] words = query.Split(' ');
 int x = 1;
 foreach(string word in words)
 {
 string param = "@param" + x;
 cmd.Parameters.AddWithValue(param, "%" + word + "%");
 x++;
 }
 }
 using (MySqlDataReader reader = cmd.ExecuteReader())
 {
 while(reader.Read())
 {
 data.Add(new UM_UserData(reader));
 }
 }
 }
 }
 return data;
}
private static string GetUsersQuery(int status, string query)
{
 string sql = "SELECT * FROM rw_db.v_sys_users";
 if (status != 3 || query != "")
 {
 sql += " WHERE 1 = 1 ";
 }
 if(status != 3)
 {
 sql += " AND `user_status` = @status ";
 }
 if(query == "") { return sql; }
 string[] words = query.Split(' ');
 string[] fields = new string[] { "user_empid", "user_firstname", "user_lastname", "user_username" };
 sql += buildSearch(words, fields);
 return sql;
}
private static string buildSearch(string[] words, string[] fields)
{
 string s = " AND((";
 int x = 1;
 foreach(string word in words)
 {
 if(x != 1) { s += "AND("; }
 string paramName = " @param" + x +" ";
 int y = 1;
 foreach(string field in fields)
 {
 if(y != 1) { s += " OR "; }
 s += field + " LIKE " + paramName + " ";
 y++;
 }
 s += " ) ";
 x++;
 }
 s += ")";
 return s;
}

---

Thank you everyone, I appreciate all of yall's advice.

Here is what I ended up with.

---

public static List<UM_UserData> GetUsers(int status, string search)
{
 List<UM_UserData> data = new List<UM_UserData>();
 UM_GetUserQuery sqlObj = new UM_GetUserQuery(status, search);
 using (MySqlConnection con = new MySqlConnection(Cfg.connectionString))
 {
 con.Open();
 using (MySqlCommand cmd = new MySqlCommand(sqlObj.sqlQuery, con))
 {
 cmd.Parameters.AddRange(sqlObj.parameters.ToArray());
 using (MySqlDataReader reader = cmd.ExecuteReader())
 {
 while (reader.Read())
 {
 data.Add(new UM_UserData(reader));
 }
 }
 }
 }
 return data;
}
internal class UM_GetUserQuery
{
 #region FIELDS
 public string sqlQuery { get; private set; }
 public List<MySqlParameter> parameters { get; private set; }
 #endregion
 #region CONSTRUCTORS
 public UM_GetUserQuery(int status, string search)
 {
 parameters = new List<MySqlParameter>();
 BuildQuery(status, search);
 }
 #endregion
 #region PRIVATE METHODS
 private void BuildQuery(int status, string search)
 {
 var builder = new StringBuilder("SELECT * FROM rw_db.v_sys_users");
 var condition = new StringBuilder();
 // IF STATUS != 3 "All Users"
 if(status != 3)
 {
 // ADD CONDITION FOR STATUS AND ADD PARAMETER TO COLLECTION
 condition.Append(" AND `user_status` = @statusParam ");
 parameters.Add(new MySqlParameter("@statusParam", status));
 }
 // IF SEARCH IS NOT EMPTY 
 if(!string.IsNullOrWhiteSpace(search))
 {
 string[] words = search.Split(' '); // SPLIT SEARCH USING SINGLE SPACE
 string[] fields = new string[] { "user_empid", "user_firstname", "user_lastname", "user_username" };
 // CREATE A NEW SEARCH OBJECT
 SearchObj searchObj = new SearchObj(words, fields);
 // APPEND SEARCH QUERY TO CONDITION
 condition.Append(searchObj.sql);
 // APPEND SEARCH PARAMETERS TO COLLECTION
 parameters.AddRange(searchObj.parameters);
 }
 // IF CONDITION IS NOT EMPTY
 if (condition.Length > 0)
 {
 // ADD CONDITION TO BUILDER
 builder.Append(" WHERE 1=1 ");
 builder.Append(condition);
 }
 // SET BUILDER TO SQL STRING
 sqlQuery = builder.ToString();
 }
 #endregion 
}
internal class SearchObj
{
 #region FIELDS
 public string sql { get; private set; }
 public List<MySqlParameter> parameters { get; private set; }
 #endregion
 #region CONSTRUCTORS
 public SearchObj(string[] words, string[] fields, bool useAND = true)
 {
 parameters = new List<MySqlParameter>();
 BuildSearch(words, fields, true);
 }
 #endregion
 #region PRIVATE METHODS
 private void BuildSearch(string[] words, string[] fields, bool useAND = true)
 {
 StringBuilder builder = new StringBuilder();
 string AndOR = useAND ? " AND(" : " OR(";
 builder.Append(" AND((");
 int x = 1;
 // LOOP THROUGH EACH SEARCH WORD IN WORDS
 foreach (string word in words)
 {
 // IF NOT FIRST WORD APPEND AND
 if (x != 1) { builder.Append(AndOR); }
 string paramName = string.Format("@param{0}", x);
 parameters.Add(new MySqlParameter(paramName, "%" + word + "%"));
 int y = 1;
 // LOOP THROUGH EACH FIELD
 foreach (string field in fields)
 {
 // IF NOT FIRST FIELD APPEND OR
 if (y != 1) { builder.Append(" OR "); }
 builder.Append($" {field} LIKE {paramName} ");
 y++;
 }
 builder.Append(")");
 x++;
 }
 builder.Append(")");
 sql = builder.ToString();
 }
 #endregion
}

• Gert Arnold: Using an ORM (with a predicate builder) is not an option?
• McGee.WIll: I suppose I could, I'm fairly new to .NET, I switched from PHP and Flex since flash is dead. I will do a little research on ORM's. I'm just not familiar with it. This was related closely with what I did in PHP
• paparazzo: Maybe just me but I would not split up the loops in three mechods
• McGee.WIll: The first 2 methods could be merged as they are directly related but I wanted to separate the third for reusability

Answer 1

In contrast to radarbob I find a StringBuilder very useful in this context.

Your code produces something like this:

SELECT * FROM rw_db.v_sys_users WHERE 1 = 1 AND user_status = @status AND((user_empid LIKE @param1 OR user_firstname LIKE @param1 OR user_lastname LIKE @param1 OR user_username LIKE @param1 ) AND(user_empid LIKE @param2 OR user_firstname LIKE @param2 OR user_lastname LIKE @param2 OR user_username LIKE @param2 ) AND(user_empid LIKE @param3 OR user_firstname LIKE @param3 OR user_lastname LIKE @param3 OR user_username LIKE @param3 ) )

which looks ok to me (except for 1 = 1 :-), but are you sure you want AND between the OR-sequences? I would say OR was a better choise.

Below find my rainy sunday afternoon version with some inline comments:

 class UserSearch
 {
 public List<UM_UserData> GetUsers(int status, string searchWords)
 {
 string connectionString = "<TODO: Connection String>";
 var parameters = GetParameters(searchWords);
 var sql = BuildSql(status, parameters);
 Console.WriteLine(sql);
 // TODO: Change SqlConnection etc. to MySqlCon... 
 // No need to fire up the connection until everything is ready...
 using (SqlConnection connection = new SqlConnection(connectionString))
 {
 connection.Open();
 using (SqlCommand command = new SqlCommand(sql, connection))
 {
 command.Parameters.AddRange(parameters.ToArray());
 var reader = command.ExecuteReader();
 List<UM_UserData> result = new List<UM_UserData>();
 while (reader.Read())
 {
 result.Add(new UM_UserData(reader));
 }
 return result;
 }
 }
 }
 List<SqlParameter> GetParameters(string searchWords)
 {
 if (string.IsNullOrWhiteSpace(searchWords))
 {
 return new List<SqlParameter>();
 }
 return searchWords
 .Split(' ')
 .Distinct()
 .Where(word => !string.IsNullOrWhiteSpace(word))
 .Select(word => word.Trim())
 .Select((word, i) => new SqlParameter($"@Param_{i}", $"%{word}%")) // TODO: change to a MySqlParameter
 .ToList();
 }
 string BuildSql(int status, List<SqlParameter> parameters)
 {
 // TODO: check for syntactical differences between MS Sql and MySql
 // Use a StringBuilder to build the strings, it is much more efficient and the code is easier to maintain and understand.
 var builder = new StringBuilder("SELECT * FROM rw_db.v_sys_users");
 if (parameters.Count > 0 || status != 3)
 {
 builder.Append(" WHERE ");
 if (parameters.Count > 0)
 {
 JoinFieldsAndParameters(builder, parameters);
 }
 // Add the status criteria as the last. It is easier to append it if necessary
 if (status != 3)
 {
 string andString = parameters.Count > 0 ? "AND" : "";
 string statusParam = $"@statusParam";
 builder.Append($" {andString} user_status = {statusParam} ");
 parameters.Add(new SqlParameter(statusParam, status));
 }
 }
 return builder.ToString();
 }
 void JoinFieldsAndParameters(StringBuilder builder, List<SqlParameter> parameters)
 {
 if (parameters.Count == 0)
 return;
 string[] fields = new string[] { "user_empid", "user_firstname", "user_lastname", "user_username" };
 foreach (var parameter in parameters)
 {
 builder.Append("(");
 foreach (var field in fields)
 {
 builder.Append($"{field} LIKE {parameter} OR ");
 }
 builder.Length -= 4; // Removes the last " OR "
 builder.Append(") AND "); // Are you sure you want AND here and not OR?
 }
 builder.Length -= 5; // Removes the last " AND "
 }
 }

• McGee.WIll: Yeah I chose to use AND to allow the user to "Drill Down" via search. For instance if the user searched for first name and found too many matches the user could then start typing the last name to drill down further, For reusability I could use an overload to switch between AND and OR. The 1=1 is there because if the status is 3 then it doesn't include status in the query, and it made it easier to build the query by appending AND to both conditions :-P
• user73941: @McGee.WIll: good point, and it's good to know that one is still one whatever choices we make ;-)

Answer 2

use string.Format() it is easier to read and far less error prone

s += string.Format("{0} LIKE {1} ", field, paramName);
string paramName = string.Format(" @param{0} ", x);

---

 string[] fields = new string[] { "user_empid", "user_firstname", "user_lastname", "user_username" };
 sql += buildSearch(words, fields)

Why pass fields as a parameter if it is constant. Put it in BuildSearch() instead.

---

Capitalize method names - C# idiom.

---

Be precise: don't say "blank string" when you mean string.Empty (a.k.a. "empty string").

This: string.Empty not: "" Eliminates uncertainty due to eyesight, font style and size, accidental spacebar, etc.

Strings are reference types and may be null. Always use String.IsNullOrWhiteSpace() - preferred over string.IsNullOrEmpty. Either one definitely preferred over myString == ""

Test string parameters for null. Calling a method on a null string reference throws an exception of course. And to simplify downstream code I like to set null strings to string.Empty

public void SomeMethod( string stringParm ){
 stringParm = stringParm ?? string.Empty; // null coalescence operator
 // ...
}

---

In BuildSearch():

 int x = 1;
foreach(string word in words)
{
 if(x != 1) { s += "AND("; }
 string paramName = " @param" + x +" ";

Return early if words is null/empty:

if (words == null || words.Length == 0) return; 
...
 int x = 1;
 s += "AND("; 
foreach(string word in words)
{
 string paramName = string.Format( " @param{0} ", x );

fields is constant, never null/empty.

y seems to never be used

So, this:

s += " OR ";
foreach(string field in fields)
{
 s += string.Format( "{0} LIKE {1} ", field, paramName );
 }

---

This does not make sense:

private static string GetUsersQuery(int status, string query)
{
 string sql = "SELECT * FROM rw_db.v_sys_users";

If this method is getting the query, why are you passing in the query?

If you are passing in the query why is a query being defined?

---

Proposed Experiment

The code structure is lousy. There is testing for status != 3 and query != null all over the place, and even repeated in a nested loop. I have to read and study a lot to figure out how the query is being built differently based on these conditionals.

Re-write the code to examine the conditionals up front and separate code for each path. This will result in some redundancy I guess but the point is to see how the code becomes clean and understandable.

Personally I'd go for clarity over some minor duplication in this case. But you should be able to DRY that up. Broader scoped common variables or via method parameters.

---

P.S.

Some readers may point out that I do not mention StringBuilder. I'm not feeling the need here. I think the proposed re-write will be far more helpful lesson in the long run.

• Paweł Hemperek: Always use String.IsNullOrWhiteSpace() - preferred over string.IsNullOrEmpty - why would you say one is preferred over second, when they are two different methods? In this case string.IsNullOrWhitespace perhaps is better, but you can't definitely say, one is better choice no matter what the case is.
• radarbob: IsNullOrWhiteSpace vs IsNullOrEmpty
• Paweł Hemperek: Yes, I saw that link - that's exactly the reason why claiming IsNullOrWhitespace is preffered over IsNullOrEmpty is simply wrong. Correct answer is - it depends.
• radarbob: Ok, it depends. I've never had strings that I intended to be only whitespace characters. And I deal a lot with fixed-length padded values that IsNullOrWhitespace handles nicely.

Answer 3

You probably don't want to search for empty strings via equating:

query != ""

Using

!String.IsNullOrEmpty(query)

or

Length.query > 0

Would result in much better code performance.

• radarbob: And, strings can be null so string.IsNullOrEmpty is always best. Note: there is a newer method string.IsNullOrWhiteSpace. Covers edge cases better.