Can I condense this if statement?

Question

if (allStatusCheckBox.Checked != true)
{
  if (assComboBox.SelectedIndex != -1 && revComboBox.SelectedIndex != -1)
  {
    dataSda = new SqlDataAdapter("SELECT DATAACTUALID WHERE ASSIGNEDSTATUS LIKE '" + statusComboBox.SelectedValue + "' AND A_ASSIGNEDTO.EMP_ID LIKE '" + assComboBox.SelectedValue + "%' AND A_TOBEREVIEWEDBY.EMP_ID LIKE '" + revComboBox.SelectedValue + "%'", patientCon);
  }
  else if (assComboBox.SelectedIndex != -1 && revComboBox.SelectedIndex == -1)
  {
    dataSda = new SqlDataAdapter("SELECT DATAACTUALID WHERE ASSIGNEDSTATUS LIKE '" + statusComboBox.SelectedValue + "' AND A_ASSIGNEDTO.EMP_ID LIKE '" + assComboBox.SelectedValue + "%'", patientCon);
  }
  else if (assComboBox.SelectedIndex == -1 && revComboBox.SelectedIndex != -1)
  {
    dataSda = new SqlDataAdapter("SELECT DATAACTUALID WHERE ASSIGNEDSTATUS LIKE '" + statusComboBox.SelectedValue + "' AND A_TOBEREVIEWEDBY.EMP_ID LIKE '" + revComboBox.SelectedValue + "%'", patientCon);
  }
  else
  {
    dataSda = new SqlDataAdapter("SELECT DATAACTUALID WHERE ASSIGNEDSTATUS LIKE '" + statusComboBox.SelectedValue + "'", patientCon);
  }       
}
else
{
    //REPEAT WITHOUT STATUSCOMBOX.SELECTED VALUE
}

Basically the point of this code is to display some information based on whether certain filters have been applied. However with my current approach every time I apply a new filter my number of if statements is growing exponentially. I am worried my code will soon become slow and hard to manage as I apply more filters. Is there a better way of achieving the same results?

Your C# code/query code is not remotely legible. I recommend just showing enough if branches to get the point across. And please wrap those queries so that they are completely visible without scrolling. — Tim Biegeleisen, Aug 08 '18 at 14:54
*I am worried my code will soon become slow* An if statement can't be sped up. This code is not going to be a performance bottleneck — Liam, Aug 08 '18 at 14:54
First off, you have a lot of repeated code. Secondly, you have glaring SQL injection vulnerabilities. I would address both of those things first and I bet you will find your actual question answered in the process — maccettura, Aug 08 '18 at 14:54
@Liam: true, nevertheless the "hard to manage" argument is valid. — Stefan, Aug 08 '18 at 14:56
@Person I would also strongly consider moving these queries into stored procedures instead of letting all this SQL sit inside, and be maintained in C# — maccettura, Aug 08 '18 at 14:58
I agree with @maccettura on that one: the argument that's it's hard to manage is mainly because your naming can be improved. Things like `assComboBox.SelectedIndex == -1` doesn't explain it's meaning on it's own and makes it hard to read. If it would say: `IsAssSelected`, it would be clearer (although it still could be improved). — Stefan, Aug 08 '18 at 15:01
worry more about the SQL injection; the performance of a few `if`s is **irrelevant** compared to a round-trip to a database server — Marc Gravell, Aug 08 '18 at 15:01
If you used an ORM like EF you could dynamically build the query which could make the code more manageable. — juharr, Aug 08 '18 at 15:09
Thanks everybody. I'm still very new to OOP and using sql so forgive me. I will research the concerns you have all raised. I appreciate you guys taking a look. — Person, Aug 08 '18 at 15:13

score 1 · Accepted Answer · answered Aug 08 '18 at 15:17

You have quite a few bigger problems to worry about with your code before worrying about some if statements. I will address all of those and you will start to see that fixing them, will fix your mess.

First, your SQL query text is basically the same across all of your queries:

SELECT 
    DATAACTUALID, 
    A_DATAACTUAL.TRIGGERPOINTSID, 
    TBLPT.PT_ID, 
    NAME as C, 
    A_ASSIGNEDTO.EMP_ID as ASSIGNEDID, 
    A_TOBEREVIEWEDBY.EMP_ID as REVIEWERID, 
    TBLPT.LASTNAME + ' ' + TBLPT.FIRSTNAME as Patient, 
    TRIGGERNAME as DESCRIPTION, 
    TRIGGERPOINTNAME as DETAIL, 
    A_ASSIGNEDTO.EMP_LASTNAME + ' ' + A_ASSIGNEDTO.EMP_FIRSTNAME as Assigned, 
    TODOBY, A_TOBEREVIEWEDBY.EMP_LASTNAME + ' ' + A_TOBEREVIEWEDBY.EMP_FIRSTNAME as Reviewer, 
    REVIEWDATE, 
    GRADE, 
    COMMENT 
FROM A_DATAACTUAL 
    INNER JOIN TBLPT ON A_DATAACTUAL.PT_ID = TBLPT.PT_ID 
    INNER JOIN A_TRIGGERPOINTS ON A_DATAACTUAL.TRIGGERPOINTSID = A_TRIGGERPOINTS.TRIGGERPOINTSID 
    INNER JOIN A_TRIGGERS ON A_TRIGGERPOINTS.TRIGGERID = A_TRIGGERS.TRIGGERID 
    INNER JOIN A_ASSIGNEDTO ON A_DATAACTUAL.ASSIGNEDTO = A_ASSIGNEDTO.EMP_ID 
    INNER JOIN A_TOBEREVIEWEDBY ON A_DATAACTUAL.TOBEREVIEWEDBY = A_TOBEREVIEWEDBY.EMP_ID 
    INNER JOIN A_STATUS ON A_DATAACTUAL.ASSIGNEDSTATUS = A_STATUS.STATUSID

You see all those joins and specific selects/aliases? This is a prime opportunity to create a SQL View, then you can query that newly created View instead of constantly doing the same exact query over and over again.

Lets say you named the view v_SomeView, all your queries would look like this:

"SELECT * FROM v_SomeView WHERE A_ASSIGNEDTO.EMP_ID LIKE ..."

"SELECT * FROM v_SomeView WHERE A_TOBEREVIEWEDBY.EMP_ID LIKE ..."

Use the power of SQL, C# should only be responsible for so much...

Second huge problem is you have SQL Injection vulnerabilities. You should never concatenate SQL queries like this. Always use parameters.

A quick example if you must use SqlDataAdapter:

dataSda = new SqlDataAdapter("SELECT * FROM v_SomeView WHERE ASSIGNEDSTATUS = @someValue", patientCon);
dataSda.SelectCommand.Parameters.Add(new SqlParameter("@someValue", assComboBox.SelectedValue));

With these two changes you have made your code more secure, you have kept it DRY and you leveraged the power of SQL.

Its good that I am learning these lessons now. So as I understand it without using parameters someone could potentially input a string that would modify my sql statement to allow them to insert or modify records or a number of other things. With parameters the statement would just treat that input as a value to search. I still can't fully wrap my head around how parameters prevent injection, but I guess that doesn't matter too much. — Person, Aug 08 '18 at 15:46
@Person You are correct with what you are understanding, as for why a good Stack Overflow question detailing why parameters work the way they do can be found [here](https://stackoverflow.com/questions/5468425/how-do-parameterized-queries-help-against-sql-injection). — maccettura, Aug 08 '18 at 15:50
Ok I think I fully understand now. Parameters prevent the possibility of escaping the select statement by using characters like ; and then writing your own sql statement. Instead the parameters treat these characters as part of the parameter. Thanks for all your help. — Person, Aug 08 '18 at 16:02

Can I condense this if statement?

1 Answers1