Clever Geek Handbook

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.

+2


source to share


7 replies


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;
+3


source


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.



+3


source


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.
+1


source


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
0


source


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.

0


source


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.

0


source


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

-1


source






More articles:


All Articles