Code Length vs Organization

Question

I have several nested insert commands. Some of the nested loops share redundant code. Should I make the redundant code its own loop, or create separate instances of the same code within each loop?

EXAMPLE (edited for clarification):

--Questions 32<->37

SET @index=0

SET @values = 'at your primary grocery store^at WalMart or Sam''s 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 values for @values and @question.

Answer 1

I use the following rule:

• If code is repeated once. think if its worth to refactor(it migth be needed again).
• If Code is repeated more than once, refactor.

Answer 2

Without knowing all the details I'd recommend using a sproc or function to encapsulate your redundant code.

Answer 3

I'd put all of this into one loop. Since the only difference is that one set of stuff is done for @foo from 0..4 and the other for 5..9, use an IF statement to switch between them, e.g.:

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

Answer 4

I agree with the commenter -- get rid of the loops. You have a powerful, set-based language and you're writing procedural code. I'd recommend re-evaluating the problem to form a solution that will work better for SQL Server (you have a community here that would help you out). While what you're doing will work (and probably does), it will be/is a maintenance headache.

Answer 5

Like all of the folks that have had to work with an existing code base, I have run into code like this. The poster is "doing the right thing"TM in that they are using loops and not cursors (Yeck!).

One way to attack the problem of needing loops is to look at what you have in the STUFF and OTHER STUFF place holders. Maybe you don't need a loop, but two different statements. Try to figure out why you need the loops and if you can insert/update a set of data instead.

In any case, the folks that have to work on the code after you have moved on to something else will thank you if go that extra yard now.

Answer 6

I'd split out my inputs into a temp table or table variable and then use that in an insert statement based on a select not a values clause.

Answer 7

OK, this should work:

**

Look Ma, No Loops!:

**

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''s 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;