Code length versus organization
I have several nested insert commands. Some of the nested loops use redundancy. Should I make the redundant code my own loop, or create separate instances of the same code in each loop?
EXAMPLE (edited for clarification):
--Questions 32<->37
SET @index=0
SET @values = 'at your primary grocery store^at WalMart or Sam' Club^at any other chain (e.g. Target, K-Mart)^in general'
IF SUBSTRING(@values, LEN(@values), 1) <> '^' SET @values = @values + '^'
WHILE (LEN(@values)<>0)
BEGIN
SET @index=CHARINDEX('^', @values)
SET @result=SUBSTRING(@values, 0, @index)
SET @values=SUBSTRING(@values, LEN(@result)+2, LEN(@values)-LEN(@result)-1)
SET @question = 'How much do you spend <b>'+@result+'</b> per trip compared to this time last year?'
SET @qnum=@qnum+1
INSERT INTO checklist_questions (
checklist_id
,checklist_question_id
,checklist_answer_category_id
,autofail_flag
,checklist_responsible_type_id
,correction_days
,checklist_question_header_id
,question
)
VALUES (
@checklist_id
,@qnum --question #
,40 --answer category id
,0 --autofail flag
,'P' --checklist_responsible_type_id
,27 --correction_days
,4 --correction_days
,@question
)
SET @i=1
WHILE (@i<=6)
BEGIN
INSERT INTO checklist_answers (
checklist_id
,checklist_question_id
,checklist_answer_category_id
,checklist_answer_type_id
,detail_flag
)
VALUES (
@checklist_id
,@qnum --question number
,38 --category
,@i --answer type
,0 --detail flag
)
SET @i=@i+1
END
END
The same pattern is repeated over and over, with different meanings for @values and @question.
OK, this should work:
**
Look Ma, no cycles !:
**
declare @checklist_id INT;
SET @checklist_id = 99 -- ??
declare @index INT, @values VARCHAR(MAX);
SET @index=0
SET @values = 'at your primary grocery store^at WalMart or Sam' Club^at any other chain (e.g. Target, K-Mart)^in general'
-- make sure all substring are bounded on both sides
IF SUBSTRING(@values, LEN(@values), 1) <> '^' SET @values = @values + '^'
IF LEFT(@values,1) <> '^' SET @values = @values + '^'
;WITH cteNumbers AS
(
SELECT ROW_NUMBER() OVER(ORDER BY object_id) as N
FROM master.sys.system_columns --just a convenient source of rows
)
, cteValues AS
(
SELECT SUBSTRING(@values, N+1, CHARINDEX('^', @values, N+1)-1) as value
, ROW_NUMBER() OVER(ORDER BY N) AS qnum
FROM cteNumbers
WHERE N < LEN(@values)
AND SUBSTRING(@values, N, 1) = '^'
)
INSERT INTO checklist_questions (
checklist_id
,checklist_question_id
,checklist_answer_category_id
,autofail_flag
,checklist_responsible_type_id
,correction_days
,checklist_question_header_id
,question
)
SELECT
@checklist_id
,qnum --question #
,40 --answer category id
,0 --autofail flag
,'P' --checklist_responsible_type_id
,27 --correction_days
,4 --correction_days
,'How much do you spend <b>'+ value +'</b> per trip compared to this time last year?'
FROM cteValues;
;WITH cteNumbers AS
(
SELECT ROW_NUMBER() OVER(ORDER BY object_id) as N
FROM master.sys.system_columns --just a convenient source of rows
)
, cteValues AS
(
SELECT SUBSTRING(@values, N+1, CHARINDEX('^', @values, N+1)-1) as value
, ROW_NUMBER() OVER(ORDER BY N) AS qnum
FROM cteNumbers
WHERE N < LEN(@values)
AND SUBSTRING(@values, N, 1) = '^'
)
INSERT INTO checklist_answers (
checklist_id
,checklist_question_id
,checklist_answer_category_id
,checklist_answer_type_id
,detail_flag
)
SELECT
@checklist_id
,qnum --question number
,38 --category
, N --answer type
,0 --detail flag
FROM cteValues AS v
CROSS JOIN (SELECT N FROM cteNumbers WHERE N <= 6) AS num;
I agree with the commenter - get rid of the loops. You have a powerful customization language and you write procedural code. I would recommend re-evaluating the issue to shape a solution that works better for SQL Server (you have a community to help you). While what you are doing will work (and probably will), it will / is a maintenance headache.
I use the following rule:
- If the code is repeated once. consider whether to refactor it (you will need it again).
- If the code is repeated more than once, refactor.
I would put it all in one loop. Since the only difference is that @foo
one set of elements is executed for 0..4 and another for 5..9, use the IF statement to switch between them, for example:
DECLARE @foo SMALLINT
DECLARE @bar SMALLINT
SET @foo=0
WHILE (@foo<10)
BEGIN
IF (@foo<5)
--STUFF
ELSE
--DIFFERENT STUFF
END
SET @bar=0
WHILE (@bar < 5)
BEGIN
--CODE
SET @bar = @bar + 1
END
SET @foo = @foo+1
END
Like all people who had to work with an existing codebase, I came across code like this. The poster is the "right thing" about using loops, not cursors (Yeck!).
One way to attack the problem requiring loops is to see what you have in the STUFF and OTHER STUFF holders. You may not need a loop, but two different operators. Try to figure out why you need loops and if you can insert / update a dataset.
Either way, people who need to work on code after you've moved on to something else will be grateful if you head out to this extra yard.
I would split my inputs into a temp variable or table variable and then use it in an insert statement depending on the select not a values clause.
Without knowing all the details, I would recommend using a sproc or a function to encapsulate your redundant code.