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.

You might want to ask your question in Code Review Stack Exchange instead — blurfus
– blurfus, Commented May 29, 2024 at 17:25
Random tips: (@color IS NULL AND Color IS NULL) OR (Color = @color) could be @color is not distinct from Color in SQL Server 2022. N'There are no products with the color ' + Coalesce( @color, N'NULL' ). Add statement terminators (;). — HABO
– HABO, Commented May 29, 2024 at 18:26
My thoughts: (1) I would drop the initial @RowCount calculation and the initial stand-alone FETCH NEXT. Just loop through the rows one-at-a-time, counting the occurrences. After the loop, if the count is zero, then take special action. (2) I never bother to check for and deallocate an existing cursor up front. I just assume the system is in a clean state to start. (Exception may be when debugging and selectively executing code fragments.) (3) If you catch an exception, perhaps you can just set a flag, drop through to cleanup logic, and finally take the appropriate return-result action. — T N
– T N, Commented May 29, 2024 at 19:39
Thank you for everyone's responses. blurfus: is there a way to automatically transfer this tread to Code Review? Habo: Thank you. Unfortunately, I am using SQL Server 2019 but I incorporated N'There are no products with the color ' + Coalesce( @color, N'NULL' ). TN: Thanks for the ProductCursor point. How come SQL Server didn't create an error? I am going to think more about your other points. — Axium7
– Axium7, Commented May 29, 2024 at 20:16

Axium7 · Accepted Answer · 2024-05-30 18:07:42Z

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;

Also be aware of the differences between LOCAL and GLOBAL cursors. Typically, you'd want to use a LOCAL cursor, to avoid errors like: "A cursor with the name 'YourCursorName' already exists". If you don't explicitly specify, typically it will default to GLOBAL (depends on database options).
Thank you for contributing to the Stack Overflow community. This may be a correct answer, but it’d be really useful to provide additional explanation of your code so developers can understand your reasoning. This is especially useful for new developers who aren’t as familiar with the syntax or struggling to understand the concepts. Would you kindly edit your answer to include additional details for the benefit of the community?

Collectives™ on Stack Overflow

Cleaning up Cursors without Spaghetti and Duplicate Code?

1 Answer 1

2 Comments

Linked

Hot Network Questions

Collectives™ on Stack Overflow

1 Answer 1

2 Comments

Linked

Related