2
\$\begingroup\$

I have a list of variables which I need to convert to LIKE statements in SQL. The final query should look like the following:

...
WHERE ([TraceableItem].[IdentificationNo] LIKE N'%015558' 
       OR [TraceableItem].[IdentificationNo] LIKE N'%123456' 
       OR [TraceableItem].[IdentificationNo] LIKE N'%654321')
...

Below is my code to do it, but I feel it is fairly inefficient / redundant. Especially features like having a bool for firstLoop, and constantly checking for items already existing. I would be very grateful for a code review and tips on how to improve - I'm fairly rusty on C#

string queryInsertString = "([TraceableItem].[IdentificationNo] LIKE ";
string lastBatchNo = batchnos.Last();
List<string> alreadyAdded = new List<string>();
bool firstLoop = true;
foreach(string item in batchnos)
            {
    string splitItem = item.Split('/')[1];
    if (!alreadyAdded.Contains(item)){
        if (!firstLoop)
        {
            queryInsertString += " OR [TraceableItem].[IdentificationNo] LIKE N'%" + splitItem + "'";
        } else
        {
            queryInsertString += "N'%" + splitItem + "'";
            firstLoop = false;
        }
        alreadyAdded.Add(item);
    }
}
\$\endgroup\$

closed as off-topic by t3chb0t, dfhwze, Toby Speight, AlexV, Peter Taylor Aug 30 at 12:38

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Lacks concrete context: Code Review requires concrete code from a project, with sufficient context for reviewers to understand how that code is used. Pseudocode, stub code, hypothetical code, obfuscated code, and generic best practices are outside the scope of this site." – t3chb0t, dfhwze, Toby Speight, AlexV, Peter Taylor
If this question can be reworded to fit the rules in the help center, please edit the question.

  • 3
    \$\begingroup\$ You're lacking some of the context here - when reviewing, we want to see the complete program or function, including the definitions of the identifiers in scope (e.g. batchnos in this question). \$\endgroup\$ – Toby Speight Aug 29 at 11:31
5
\$\begingroup\$

Using String.Join makes this kind of string operation a breeze:

// DON'T USE THIS - SQL INJECTION
string predicates = string.Join(" OR ", 
        batchNos.Select(item => item.Split('/')[1])
                .Distinct()
                .Select(splitItem => $"[TraceableItem].[IdentificationNo] LIKE N'%{splitItem}'"));
string queryInsertString = "(" + predicates + ")";

You'll notice the all capital warning not to use that code. It's not just because I haven't even checked if it compiles/works, it's because this kind of thing introduces a SQL Injection Vulnerability.

What you need to do is read up on using SqlParameters in C#. You'll want to generate SQL that looks more like:

[TraceableItem].[IdentificationNo] LIKE @param1 OR [TraceableItem].[IdentificationNo] LIKE @param2

You then need to create a SqlParameter instance for each one and add that to the SqlCommand before you execute it. That's assuming you're using Ado.Net, if you're using a framework you'll have to look up the right way of doing it in the one you're using.

The danger is that if someone passes a batch number of "a/'); delete from Users; --", you'll end up creating a query that looks like:

WHERE ([TraceableItem].[IdentificationNo] LIKE N''); delete from Users; -- ...

You do not want a user to be able to do that!

\$\endgroup\$
  • \$\begingroup\$ Thank you very much. SQL Injection wasn't something I'd thought about \$\endgroup\$ – Harry Adams Sep 2 at 8:49
4
\$\begingroup\$

Your SQL query is never going to be efficient because it has wildcard at the start, and is not Sargable (LIKE N'%015558').

Assuming your database allows it, what you should do in this scenario is to create a PERSISTED computed SQL column which is set to REVERSE(IdentificationNo), and create an index on that column.

Combine this with the advice from @RobH, and make your query something similar.

WHERE [ReversedIdentificationNo] LIKE + splitItem.Reverse() + '%'

This way, your query will use an index seek (instead of an index scan) and potentially be magnitudes faster.

EDIT: With a variable number of IdentificationNo that you intend of searching, you run into a cache pollution issue. For every distinct number of parameters, your SQL engine will not be able to reuse an existing query cache plan. This is very common issue when dynamically generating SQL statements (especially without SQL paramters).

To avoid this, you could use a temp table / table type.

CREATE TYPE Ids AS TABLE ( ID varchar(128) PRIMARY KEY )

Then, instead of constructing a bunch of OR statements, you just insert all your splitItem into this temp table. Finally, your query looks something like the following.

WHERE EXISTS (
    SELECT 1 
    FROM @Ids
    WHERE [ReversedIdentificationNo] LIKE [Id] + '%'
)
\$\endgroup\$

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