0

My code actually works, I don't need help with that. What I would like to know if what I have done is considered acceptable.

In one particular part of a T-SQL script I am writing I have to run almost similar insert statements about 20 times. Only a portion of the WHERE clause is different in each case. Wanting to loop, rather than have 20 almost identical inserts, I use a WHILE loop to run some dynamic SQL and I store the portion of the WHERE clause that differs in the database. Works like a charm. It's worth noting that the INSERT statements in this case may vary in number or in content and I felt this solution allowed a way to deal with that rather simply.

When showing one of my peers at work this solution to the problem, his one eyebrow went up and he looked at me as though I was growing a new head. He suggested that there was a better way. That may be and with me being the junior I'll humbly accept it. But, I did want to ask the community if this seems like a weird, unprofessional or against general standards / best practices.

I can post the code if needed but for the purposes hopefully I have given you enough to comment one way or the other.

TIA

Edit--

OK, as requested here is the code. I won't try to explain it as it's a can of worms but here it is.

DECLARE @varOfferId INT = 1
DECLARE @MaxOfferId INT = (SELECT COUNT(DISTINCT offer_id) FROM obp.CellCodes_Offers
DECLARE @SQLWhereClause VARCHAR(1000)
DECLARE @SQLStatement VARCHAR(1000)

WHILE @varOfferId <= @MaxOfferId
BEGIN
SET @SQLWhereClause = (SELECT where_clause FROM obp.Offers WHERE offer_id = @varOfferId)
SET @SQLStatement = 
'INSERT INTO obp.Offers_Contacts ' +
    'SELECT DISTINCT o.contact_id, ' + CONVERT(VARCHAR(2), @varOfferId) +
    ' FROM obp.Onboarding AS o
    WHERE ' + @SQLWhereClause +
    ' AND o2.contact_id = o.contact_id)
      AND ' + CONVERT(VARCHAR(2), @varOfferId) + ' IN(
      SELECT cc.offer_id
      FROM obp.CellCodes_Offers AS cc
      WHERE cc.cellcode = o.cellcode)'

EXECUTE (@SQLStatement)
SET @varOfferId = @varOfferId + 1
END

So, it seems that the consensus thus far is thinking this is not a good idea. OK, I'm good with that. But I'm not sure I agree that it is easier from a maintenance standpoint. Right now my code looks at the 'Offers' table, gets the row count and loops that many times. If they add more offers going forward (or reduce the offers) all I have to do is an INSERT (or DELETE) and include the offer with the appropriate WHERE clause and we are on our way. Alternatively, if I write all the individual INSERTS if they add or remove I've got to touch the code which means testing/qa. Thoughts?

However, I do agree with several other points so I guess I'll be going back to the drawing board tomorrow!

marc_s
  • 675,133
  • 158
  • 1,253
  • 1,388
  • 1
    It looks pretty bizarre to me. I'm surprised your colleague's eyebrow stayed on his forehead. There's bound to be a better way - post your code. –  Oct 08 '13 at 02:26
  • OK. Posted my code. Interested to see what you would suggest. – Joel LaRusic Oct 08 '13 at 05:08
  • Can you include a couple of different examples of the WHERE clauses you are storing in the table? It's probably possible to modify that other table to be an 'offer features' table or similar, and then when a new offer comes along, you populate the 'offer features' table with the new offer details. – Dave Hilditch Oct 08 '13 at 12:28
  • For the record, now that I see your use-case, I agree that writing out individual insert statements is not going to be a good idea, but still, you should be able to load a 'offerfeatures' table with all the relevant details and then have a single query to join to that table and produce your insert command. – Dave Hilditch Oct 08 '13 at 12:31
  • FWIW, I've implemented systems that included `SELECT` queries stored in a table. The could include _snippets_ which my code would replace prior to execution, e.g. `select * from Sales where UserId = {UserId}`. The code to execute it would replace `{UserId}` with the appropriate value for the current user, along with any other snippets delimited by curly braces. As long as I controlled adding queries to the table and the assortment of supported snippets I could avoid SQL injection issues. Don't know if there might be something of value to you there. – HABO Oct 08 '13 at 13:59
  • 1
    Tip: Using `COUNT` to determine a maximum value is curious. You start counting from 1 by 1's to the number of offers. After some offers come and go and the current ones are 3, 7, 15 and 42 you may be disappointed. This might be one of the cases where a `CURSOR` is the right choice. – HABO Oct 08 '13 at 14:02
  • Here are a couple of the where clauses that I have stored in the DB. 'NOT EXISTS (SELECT 1 FROM obp.Products p JOIN obp.Onboarding AS o2 ON p.product_id = o2.product_id WHERE p.product_class = 5' and another, 'NOT EXISTS (SELECT 1 FROM obp.Products p JOIN obp.Onboarding AS o2 ON p.product_id = o2.product_id WHERE (p.product_name = 'Mutual Fund' or p.Product_Name = 'Mutual Funds')' The overall logic is 'If they don't have product A, offer them product A. I will look into the possibility of storing other relevant details of the offer rather than the actual code. Thanks. – Joel LaRusic Oct 08 '13 at 15:57

2 Answers2

2

Pros:

  1. You've kept your code shorter, saved some time

Cons:

  1. You are now susceptible to SQL Injection
  2. Your DB code is now half in the DB and half in the table - this will make maintenance harder for whoever maintains your code.
  3. Debugging is going to be difficult.

If you have to write 20 different statements, it may be possible to autogenerate them using a very similar WHILE LOOP to the one you've already made.

e.g.

SELECT 'insert into mytable (x,y,z) from a join b on a.x = b.x ' + wherecolumn
from wheretable

This would give you the code you need to paste into your stored procedure. You could even keep that statement above in the stored procedure, commented out, so others may re-use it in future if column structures change.

For the best post I've ever seen on dynamic SQL check out Erland Somerskog's page here.

Community
  • 1
  • 1
Dave Hilditch
  • 5,008
  • 3
  • 25
  • 35
0

I think recording the difference in a database is relatively less straightforward and less convenient to modify afterwards. I would just write a script to do this, and write the conditions in the script directly.

For example, in Python you may write something like this.

    import MySQLdb
    import MySQLdb.cursors

    field_value_pairs = {'f1':'v1', 'f2':'v2', 'f3':'v3'}  # this is where you could modify to meet your different cases

    db = MySQLdb.connect(host=host_name, user=user_name, passwd=password, \
        unix_socket=socket_info)
    cursor = db.cursor()
    db.select_db(db_name)

    for field in field_value_pairs.keys():
      cursor.execute("""INSERT INTO tbl_name (%s) VALUES (%s)""", field, field_value_pairs[field])
      db.commit()

    cursor.close()
    db.close()
user1036719
  • 966
  • 2
  • 12
  • 29