Cleaning up Cursors without Spaghetti and Duplicate Code?

Question

I am using GoTo for Closing and Deallocating the Cursor. Is there a better way to write my code? Note, this code is for learning how to use Cursors and output Parameters.

I am new to TSQL, so feel free to comment on other items. For example, I was looking for a stored Procedure that will close a Cursor if it is Open but I didn't find anything via Google Search.

Thank you

-- Find the MAX, MIN, Avg for each Color in the Products table using Cursor

USE [AdventureWorks2019]
GO
/****** Object:  StoredProcedure [dbo].[WithCursor]    Script Date: 5/29/2024 11:30:17 AM ******/
SET ANSI_NULLS ON
GO
SET QUOTED_IDENTIFIER ON
GO
ALTER     PROC [dbo].[WithCursor]
(
    @color VARCHAR(20) = NULL,
    @MinStock INT OUTPUT,
    @MaxStock INT OUTPUT,
    @AvgStock INT OUTPUT
)
AS

SET NOCOUNT ON

BEGIN TRY
    
    Declare @recordCount int = ( SELECT Count(*) 
        FROM Production.Product 
        WHERE (@color IS NULL AND Color IS NULL) OR (Color = @color))

    -- ******************** NO Records Exist ********************
    IF @recordCount = 0
    Begin

        DECLARE @errorMessage NVARCHAR(200);
        IF @color IS NULL
            SET @errorMessage = N'There are no products with the color NULL';
        ELSE
            SET @errorMessage = CONCAT(N'There are no products with the color ', @color);

        RAISERROR(@errorMessage, 11, 1);
        RETURN -2; -- no such color

    End

    -- ******************** Open the Cursor for Products ********************
    DECLARE @level int;
    IF EXISTS (
        SELECT 1 
        FROM sys.dm_exec_cursors(0) 
        WHERE name = 'pCursor'
    )
    BEGIN
        CLOSE pCursor
        DEALLOCATE ProductCursor;
    END

    DECLARE pCursor CURSOR FOR 
            SELECT SafetyStockLevel
            FROM Production.Product
            WHERE (@color IS NULL AND Color IS NULL) OR (Color = @color);
        
    --Open the Cursor.  We know there is at least 1 record from the above check.
    OPEN pCursor  
    FETCH NEXT FROM pCursor INTO @level

    -- ******************** 1 Record Exist ********************
    IF @recordCount = 1
    Begin

        Set @MinStock = @level
        Set @MaxStock = @level
        Set @AvgStock = @level

        GoTo Cleanup

    End
    -- ******************** 2 or More Records Exist ********************
    
    Declare @SumLevel int
    Declare @MinLevel int
    Declare @MaxLevel int

    Set @SumLevel = @level
    Set @MinLevel = @level
    Set @MaxLevel = @level

    --Move to the second record
    FETCH NEXT FROM pCursor  INTO @level

    While @@FETCH_STATUS = 0
    Begin
        
        Set @SumLevel = @SumLevel + @level

        If @level < @MinLevel
            Set @MinLevel = @level

        If @level > @MaxLevel
            Set @MaxLevel = @level

        FETCH NEXT FROM pCursor  INTO @level
    End
        
    
    Set @MinStock = @Minlevel
    Set @MaxStock = @Maxlevel
    Set @AvgStock = @SumLevel / @recordCount
    
   GoTo Cleanup

END TRY

BEGIN CATCH
    THROW;
    
    --Cleanup
    IF EXISTS (
        SELECT 1 
        FROM sys.dm_exec_cursors(0) 
        WHERE name = 'pCursor'
    )
    BEGIN
        CLOSE pCursor
        DEALLOCATE ProductCursor;
    END
    
    RETURN -1;
END CATCH;

--Success
Cleanup:
CLOSE pCursor
DEALLOCATE pCursor
Return 0

I can duplicate the code and that will remove the GoTo but I am trying not to Duplicate code as much as possible.

Answer 1

I refactored the code with everyone's suggestions. Thank you everyone. It was very helpful to read your feedback. Going forward I will use Local Cursors.

    USE [AdventureWorks2019];
GO
/****** Object:  StoredProcedure [dbo].[WithCursor]    Script Date: 5/29/2024 11:30:17 AM ******/
SET ANSI_NULLS ON;
GO
SET QUOTED_IDENTIFIER ON;
GO

CREATE OR ALTER PROC [dbo].[WithCursor]
(
    @color VARCHAR(20) = NULL,
    @MinStock INT OUTPUT,
    @MaxStock INT OUTPUT,
    @AvgStock INT OUTPUT
)
AS

SET NOCOUNT ON;

BEGIN TRY

    -- ******************** Open the Cursor for Products ********************

    DECLARE @level INT;

    --Local Cursor doesn't need to be deallocated
    DECLARE @pCursor CURSOR;

    SET @pCursor = CURSOR READ_ONLY FORWARD_ONLY
      FOR 
        SELECT SafetyStockLevel
          FROM Production.Product
         WHERE Color = @color OR (@Color IS NULL AND Color IS NULL);

    -- Open the Cursor. 
    OPEN @pCursor;
    FETCH NEXT FROM @pCursor INTO @level;

    -- ******************** Empty Recordset ********************
    IF @@FETCH_STATUS <> 0
    BEGIN 
        DECLARE @errorMessage NVARCHAR(200);
        SET @errorMessage = N'There are no products with the color ' + COALESCE(@color, N'NULL');
        RAISERROR(@errorMessage, 11, 1);
        RETURN -2; -- no such color
    END;

    -- ******************** 1 or More Records Exist ********************

    DECLARE @SumLevel INT = 0;
    DECLARE @MinLevel INT = @level;
    DECLARE @MaxLevel INT = @level;
    DECLARE @recordCount INT = 0;

    WHILE @@FETCH_STATUS = 0
    BEGIN
        SET @SumLevel = @SumLevel + @level;

        IF @level < @MinLevel
            SET @MinLevel = @level;

        IF @level > @MaxLevel
            SET @MaxLevel = @level;

        FETCH NEXT FROM @pCursor INTO @level;

        SET @recordCount = @recordCount + 1;
    END;

    --Set the Output Parameters
    SET @MinStock = @MinLevel;
    SET @MaxStock = @MaxLevel;
    SET @AvgStock = @SumLevel / @recordCount;


END TRY

BEGIN CATCH

    THROW;
    RETURN -1;

END CATCH;