2

this is my first question, hopefully I do things correctly. Also to preface, I'm a programming noob tasked with developing and fixing some code where I work since the programmer we've had until now left. And if done quite well for now with my limited knowledge.

I'm trying to fix as much as possible the structure of some of the code in various programs, and part of it is converting all (or most) the SqlCommand to use parameters. Where I'm stuck is when the code is calling string variables using the ternary operator (?:) and using multiple of those strings concatenated to build the full SQL command string.

For example:

string RecogidosCoresString    = seeCORES ? " and [PICKUPTYPE] = 2 " : " ";
string RecogidosSinCoresString = seeSINCORES ? " and [PICKUPTYPE] <> 2 " : " ";
string RecogidosHolds          = seeHOLDS ? " and [PICKUPSTATUS] = 1 " : " and [PICKUPSTATUS] > 1 ";

string stringreadHeader = "SELECT * FROM [RecogidosHeader] WHERE [ENTRYDATE] >= @EntryDate1 AND [ENTRYDATE] < @EntryDate2 AND ([PICKUPNMBR] = @RecogidoID OR CUSTNMBR = @CustNumb )";

SqlCommand readHeader = new SqlCommand(stringreadHeader + RecogidosHolds + RecogidosCoresString + RecogidosSinCoresString + "Order By [PICKUPNMBR] ASC", AppsConnect);

readHeader.Parameters.Add("@EntryDate1", SqlDbType.DateTime).Value = dateTimePicker1.Value.ToShortDateString();
readHeader.Parameters.Add("@EntryDate2", SqlDbType.DateTime).Value = dateTimePicker2.Value.AddDays(1).ToShortDateString();
readHeader.Parameters.Add("@RecogidoID", SqlDbType.VarChar, 50).Value = textBox1.Text;
readHeader.Parameters.Add("@CustNumb", SqlDbType.VarChar, 50).Value = textBox1.Text;

As you can see, I've been able to replace a lot of the command with parameters, I just don't know how to implement it with the other string variables. The command above works as is, but if possible, I'd like to replace the rest of the strings with parameters instead of doing the concatenating I have currently. I have other applications using similar structures, but hopefully if I can see how to fix this one I'll be able to apply it to the rest.

Not sure if the way I have been going about it is good current practice, but I can't over-complicate it either.

The string variables get their values depending on some checkboxes' status.

Any help would be appreciated. I've seen similar questions, but I haven't been able to grasp the solutions properly, so hopefully with a personal example I'll be able to make it work.

Or, maybe it's fine as it is and I shouldn't change it?

Edit: Did a preliminary testing of the answer @pwilcox gave and it seems to be working, at the very least the SQL query is going through and I have no errors, and the results seem like the ones I'm expecting. If correct, then hopefully now I'll be able to apply this logic to the other situations that need improving.

Thanks!

4
  • 1
    Why are you telling it that EntryDate1 and EntryDate2 are SqlDbType.DateTime but then passing a string to it? dateTimePicker1.Value and dateTimePicker2.Value.AddDays(1) are both perfectly good DateTime types Commented Feb 20, 2020 at 19:04
  • 3
    For your first question, this isn't that bad. Welcome to SO. Commented Feb 20, 2020 at 19:05
  • @ŇɏssaPøngjǣrdenlarp No clue why that is written that way (don't recall if it was originally like that or if I changed it for some reason). It currently works as is, but will try making those changes later and testing it out. Thanks. Commented Feb 20, 2020 at 19:20
  • @ŇɏssaPøngjǣrdenlarp Did some testing, and the query doesn't work without the .ToShortDateString() portion for the date. Don't know why that is, but it needs it to work. Commented Mar 4, 2020 at 18:55

1 Answer 1

1

Handle the different possibilities of seeCORES, seeSINCORES and seeHOLDS with or statements inside the SQL statement itself. In reading below it may help to remember that in sql server, and is processed first before or.

string stringreadHeader = @"
    select      * 
    from        recogidosHeader
    where       entrydate >= @EntryDate1 
    and         entrydate < @EntryDate2 
    and         (pickupnmbr = @RecogidoID or custnmbr = @CustNumb)

    and         (@seeCORES = 1 and pickuptype = 2 or @seeCORES = 0)
    and         (@seeSINCORES = 1 and pickuptype <> 2 or @seeSINCORES = 0)
    and         (
                       @seeHOLDS = 1 and pickupstatus = 1
                    or @seeHOLDS = 0 and pickupstatus > 1
                )

    order by    pickupnmbr
";

...
readHeader.Parameters.Add("@seeCORES", SqlDbType.Bit).Value = seeCORES ? 1 : 0;
readHeader.Parameters.Add("@seeHOLDS", SqlDbType.Bit).Value = seeHOLDS ? 1 : 0;

I don't know if you need the ? 1 : 0 part, conversion from boolean to bit may just work with the variables as they are.

Just to note though, in your original code, I believe setting seeCORES and seeSINCORES both to true will give very similar output to setting them both to false. The only difference will be that setting them both to false will give MORE records if there are any null values in pickuptype. Is this the intended behavior? (rhetorical question).

Sign up to request clarification or add additional context in comments.

8 Comments

Follow this fiddle to see a working example with a different schema, but using the same concept.
OK, I'll try to make sense of this and report back. Did you forget the variable seeSINCORES? Sorry, just saw what you wrote about that.
No, that's accounted for in my narrative, but I think I can see why I might have been wrong to exclude it. Stand by while I make an edit.
Thanks, I'm not sure how to make it work as I want yet, so I'll see when I can test it, but I'm getting a better understanding on the logic behind it. As for your last sentence of your answer, can you elaborate a bit since I don't understand, in particular what the alternative would be ("with the variables as they are"). Sorry for my noobness.
@MasterOfNothing, there are too many open questions to answer that with much confidence. But I can advise you look at the Execution plan (SSMS -> Query -> Include Actual Execution Plan) to investigate. Also, I would say that there are a lot of or statements in the filtering. So if you have the ability to change how your check boxes work, where, for instance, the user can check both to see all data, one to see some of it, and none to see nothing, I think that would simplify your statement and it may very well run faster.
|

Your Answer

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

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.