Problem
The @GradeOverrides and @GradeWeights parameter parsing blocks (lines ~475–555) use explicit CURSOR LOCAL FAST_FORWARD loops over STRING_SPLIT results. This pattern is anti-idiomatic when set-based alternatives are available, adds visual complexity to the parameter validation section, and is harder to maintain correctly (cursor lifecycle management, CONTINUE / FETCH NEXT in each error branch).
The cursor approach also has a subtle bug: if RAISERROR severity is raised to 16 inside the cursor loop for validation errors (it isn't currently, errors accumulate in @errors), the cursor would not be cleaned up. Even at severity 10 (the current approach), any future refactoring that adds an early RETURN inside the loop risks leaving the cursor open.
Additionally, the cursor declaration uses undeclared column aliases (@go_pair) populated from STRING_SPLIT's value column, which is fine but relies on STRING_SPLIT always returning a single column named value — this is an implicit contract not documented in the code.
Evidence
Grade override cursor block (lines ~478–520):
DECLARE go_cursor CURSOR LOCAL FAST_FORWARD FOR
SELECT LTRIM(RTRIM(value)) FROM STRING_SPLIT(@GradeOverrides, N',') WHERE LTRIM(RTRIM(value)) <> N'';
OPEN go_cursor;
FETCH NEXT FROM go_cursor INTO @go_pair;
WHILE @@FETCH_STATUS = 0
BEGIN
IF CHARINDEX(N'=', @go_pair) = 0
BEGIN
SET @errors = @errors + N'@GradeOverrides: invalid pair ...';
FETCH NEXT FROM go_cursor INTO @go_pair;
CONTINUE;
END;
...
FETCH NEXT FROM go_cursor INTO @go_pair;
END;
CLOSE go_cursor;
DEALLOCATE go_cursor;The same pattern is repeated verbatim for @GradeWeights (lines ~525–565).
Proposed Fix
Replace both cursor blocks with set-based CROSS APPLY / STRING_SPLIT parsing using a single inline table expression. Error accumulation can still be done via STRING_AGG or concatenation from a derived table:
-- @GradeOverrides: set-based parse
;WITH override_pairs AS (
SELECT
pair = LTRIM(RTRIM(value)),
cat = UPPER(LTRIM(RTRIM(LEFT(LTRIM(RTRIM(value)), CHARINDEX('=', LTRIM(RTRIM(value))) - 1)))),
val = UPPER(LTRIM(RTRIM(SUBSTRING(LTRIM(RTRIM(value)), CHARINDEX('=', LTRIM(RTRIM(value))) + 1, 50))))
FROM STRING_SPLIT(@GradeOverrides, N',')
WHERE LTRIM(RTRIM(value)) <> N''
AND CHARINDEX('=', LTRIM(RTRIM(value))) > 0
)
SELECT
@override_completion = MAX(CASE WHEN cat = N'COMPLETION' THEN CASE WHEN val = 'IGNORE' THEN 'X' ELSE LEFT(val,1) END END),
@override_reliability = MAX(CASE WHEN cat = N'RELIABILITY' THEN CASE WHEN val = 'IGNORE' THEN 'X' ELSE LEFT(val,1) END END),
...
FROM override_pairs
WHERE cat IN (N'COMPLETION', N'RELIABILITY', N'SPEED', N'WORKLOAD')
AND val IN (N'A', N'B', N'C', N'D', N'F', N'IGNORE');Error messages for invalid pairs can be accumulated with a separate STRING_AGG over rows that fail the validation predicates.
Impact
Low. The cursor approach works correctly in practice (FAST_FORWARD, small input sets). The impact is primarily code quality, maintainability, and adherence to the procedure's own T-SQL style (which is otherwise consistently set-based). The subtle cursor leak risk on future refactoring is the most concrete functional concern.
Problem
The
@GradeOverridesand@GradeWeightsparameter parsing blocks (lines ~475–555) use explicitCURSOR LOCAL FAST_FORWARDloops overSTRING_SPLITresults. This pattern is anti-idiomatic when set-based alternatives are available, adds visual complexity to the parameter validation section, and is harder to maintain correctly (cursor lifecycle management,CONTINUE/FETCH NEXTin each error branch).The cursor approach also has a subtle bug: if
RAISERRORseverity is raised to 16 inside the cursor loop for validation errors (it isn't currently, errors accumulate in@errors), the cursor would not be cleaned up. Even at severity 10 (the current approach), any future refactoring that adds an earlyRETURNinside the loop risks leaving the cursor open.Additionally, the cursor declaration uses undeclared column aliases (
@go_pair) populated fromSTRING_SPLIT'svaluecolumn, which is fine but relies onSTRING_SPLITalways returning a single column namedvalue— this is an implicit contract not documented in the code.Evidence
Grade override cursor block (lines ~478–520):
DECLARE go_cursor CURSOR LOCAL FAST_FORWARD FOR SELECT LTRIM(RTRIM(value)) FROM STRING_SPLIT(@GradeOverrides, N',') WHERE LTRIM(RTRIM(value)) <> N''; OPEN go_cursor; FETCH NEXT FROM go_cursor INTO @go_pair; WHILE @@FETCH_STATUS = 0 BEGIN IF CHARINDEX(N'=', @go_pair) = 0 BEGIN SET @errors = @errors + N'@GradeOverrides: invalid pair ...'; FETCH NEXT FROM go_cursor INTO @go_pair; CONTINUE; END; ... FETCH NEXT FROM go_cursor INTO @go_pair; END; CLOSE go_cursor; DEALLOCATE go_cursor;The same pattern is repeated verbatim for
@GradeWeights(lines ~525–565).Proposed Fix
Replace both cursor blocks with set-based
CROSS APPLY / STRING_SPLITparsing using a single inline table expression. Error accumulation can still be done viaSTRING_AGGor concatenation from a derived table:Error messages for invalid pairs can be accumulated with a separate
STRING_AGGover rows that fail the validation predicates.Impact
Low. The cursor approach works correctly in practice (FAST_FORWARD, small input sets). The impact is primarily code quality, maintainability, and adherence to the procedure's own T-SQL style (which is otherwise consistently set-based). The subtle cursor leak risk on future refactoring is the most concrete functional concern.