HiveBrain v1.2.0
Get Started
← Back to all entries
patternsqlMinor

SQL Stored Procedure Get Distinct and Update

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
codereviewsqlsql-servert-sqlstackoverflowcursor
storeddistinctupdatesqlproceduregetand

Problem

The idea is to show how many times a user (by EmployeeID) is in the TblTableList and then update the TblTableStatusCount

I have been learning Fetch this week so wanted to ensure I have gone down the right path and this is a small database, can/should I use this for any size?

Code:

```
USE [database]
GO

SET ANSI_NULLS ON
GO
SET QUOTED_IDENTIFIER ON
GO
Alter PROCEDURE [dbo].[spSummary]
-- Add the parameters for the stored procedure here

AS

BEGIN

DECLARE @StatusTotal AS Int
DECLARE @Status0 AS Int
DECLARE @Status1 AS Int
DECLARE @Status2 AS Int
DECLARE @Status3 AS Int

--Cursor to loop through
DECLARE @EmployeeID AS NvarChar(10);
DECLARE propCurs CURSOR FOR
SELECT EmployeeID FROM [TblTableList]

--Open Loop Code
OPEN propCurs;
FETCH NEXT FROM propCurs INTO @EmployeeID
WHILE @@FETCH_STATUS = 0
--Now do repeated code

BEGIN

-- SELECT @StatusTotal = COUNT(ID) FROM [TblTableCountFrom] WHERE EmployeeID = @EmployeeID
--SELECT @Status0 = COUNT(DISTINCT(ID)) FROM [TblTableCountFrom] WHERE EmployeeID = @EmployeeID AND CurrentAlertStatus = '0'
--SELECT @Status1 = COUNT(DISTINCT(ID)) FROM [TblTableCountFrom] WHERE EmployeeID = @EmployeeID AND CurrentAlertStatus = '1'
--SELECT @Status2 = COUNT(DISTINCT(ID)) FROM [TblTableCountFrom] WHERE EmployeeID = @EmployeeID AND CurrentAlertStatus = '2'
--SELECT @Status3 = COUNT(DISTINCT(ID)) FROM [TblTableCountFrom] WHERE EmployeeID = @EmployeeID AND CurrentAlertStatus = '3'

with cnt as (
SELECT
1 as cntAll
, case WHEN CurrentAlertStatus = '0' then 1 else 0 end as cnt1
, case WHEN CurrentAlertStatus = '1' then 1 else 0 end as cnt2
, case WHEN CurrentAlertStatus = '2' then 1 else 0 end as c

Solution

Consistency

You have this:

with cnt as ( 
       SELECT 
         1 as cntAll 
         , case WHEN CurrentAlertStatus = '0' then 1 else 0  end as cnt1
         , case WHEN CurrentAlertStatus = '1' then 1 else 0  end as cnt2
         , case WHEN CurrentAlertStatus = '2' then 1 else 0  end as cnt3
         , case WHEN CurrentAlertStatus = '3' then 1 else 0  end as cnt4
       from [TblTableCountFrom] where EmployeeID = @EmployeeID


And this:

SELECT @StatusTotal = sum(cntAll),  
      @Status0  = sum(cnt1),
      @Status1  = sum(cnt2),
      @Status2  = sum(cnt3),
      @Status3  = sum(cnt4)
      FROM cnt;


While both styles of formatting are perfectly acceptable, it's better to sick with the same throughout your code for readability.

Same for this:

DECLARE @StatusTotal AS Int


And this:

DECLARE @EmployeeID  AS NvarChar(10);


It works with or without a semicolon in this case, try to stick to one method.

The elephant in the room

Cursors are very slow. Especially using a bunch of variables with it. I understand you were experimenting with a function of SQL you are new to, and that's great. We all do that, play with our shiny new toy.

However, for something that's production and that will get called regularly, efficiency is key. Your clients won't care how cool your code looks, especially if it takes longer to run.

It is very rare that you actually need a cursor. Think about it this way. If you have 100 employees in your table, then it is running the query 100 times instead of once. Imagine 5000 or 50000 employees how long this would take.

I commented out all sections I think you could safely dispense with for better efficiency:

USE [database] 
    GO
    SET ANSI_NULLS ON
    GO
    SET QUOTED_IDENTIFIER ON
    GO
    Alter PROCEDURE [dbo].[spSummary]
        -- Add the parameters for the stored procedure here
    AS
    BEGIN

    DECLARE @StatusTotal AS Int
    DECLARE @Status0 AS Int
    DECLARE @Status1 AS Int
    DECLARE @Status2 AS Int
    DECLARE @Status3 AS Int

    --Cursor to loop through
    DECLARE @EmployeeID  AS NvarChar(10);

    /* DECLARE propCurs CURSOR FOR
    SELECT  EmployeeID FROM [TblTableList]

    --Open Loop Code
    OPEN propCurs;
    FETCH NEXT FROM propCurs INTO @EmployeeID
    WHILE @@FETCH_STATUS = 0  
    --Now do repeated code

      BEGIN */

     -- SELECT @StatusTotal = COUNT(ID) FROM [TblTableCountFrom] WHERE EmployeeID = @EmployeeID  
     -- SELECT @Status0 =  COUNT(DISTINCT(ID)) FROM [TblTableCountFrom] WHERE EmployeeID = @EmployeeID AND CurrentAlertStatus = '0'
     -- SELECT @Status1 =  COUNT(DISTINCT(ID)) FROM [TblTableCountFrom] WHERE EmployeeID = @EmployeeID AND CurrentAlertStatus = '1'
     -- SELECT @Status2 =  COUNT(DISTINCT(ID)) FROM [TblTableCountFrom] WHERE EmployeeID = @EmployeeID AND CurrentAlertStatus = '2'
     -- SELECT @Status3 =  COUNT(DISTINCT(ID)) FROM [TblTableCountFrom] WHERE EmployeeID = @EmployeeID AND CurrentAlertStatus = '3'

    with cnt as ( 
       SELECT 
         1 as cntAll 
         , case WHEN CurrentAlertStatus = '0' then 1 else 0  end as cnt1
         , case WHEN CurrentAlertStatus = '1' then 1 else 0  end as cnt2
         , case WHEN CurrentAlertStatus = '2' then 1 else 0  end as cnt3
         , case WHEN CurrentAlertStatus = '3' then 1 else 0  end as cnt4
       from [TblTableCountFrom] where EmployeeID = @EmployeeID
    )
    SELECT @StatusTotal = sum(cntAll),  
      @Status0  = sum(cnt1),
      @Status1  = sum(cnt2),
      @Status2  = sum(cnt3),
      @Status3  = sum(cnt4)
      FROM cnt;
        -- Check if ID Exists

        --HERE NEED TO CHECK IF EXISTS OR SHOULD I WIPE THE TABLE AT THE START? 
        IF EXISTS (SELECT EmployeeID FROM [TblTableCount] WHERE @EmployeeID = EmployeeID)

            UPDATE [TblTableCountFrom]Count 
            SET StatusTotalCount = @StatusTotal, Status0 = @Status0, Status1 = @Status1, Status2 = @Status2, Status3 = @Status3
            WHERE EmployeeID = @EmployeeID
          Else
            INSERT INTO [TblTableCount] 
            (EmployeeID, StatusTotalCount, Status0, Status1 , Status2, Status3) VALUES (@EmployeeID, @StatusTotal,@Status0, @Status1, @Status2,@Status3 )

        /* END

        -- Now we bookend the loop code

        FETCH NEXT FROM propCurs INTO @EmployeeID */

    END

    --Tidy up and close loop code
    /* CLOSE propCurs;
    DEALLOCATE propCurs; */


Also!

Do what @ckuhn203 said about a view.

Code Snippets

with cnt as ( 
       SELECT 
         1 as cntAll 
         , case WHEN CurrentAlertStatus = '0' then 1 else 0  end as cnt1
         , case WHEN CurrentAlertStatus = '1' then 1 else 0  end as cnt2
         , case WHEN CurrentAlertStatus = '2' then 1 else 0  end as cnt3
         , case WHEN CurrentAlertStatus = '3' then 1 else 0  end as cnt4
       from [TblTableCountFrom] where EmployeeID = @EmployeeID
SELECT @StatusTotal = sum(cntAll),  
      @Status0  = sum(cnt1),
      @Status1  = sum(cnt2),
      @Status2  = sum(cnt3),
      @Status3  = sum(cnt4)
      FROM cnt;
DECLARE @StatusTotal AS Int
DECLARE @EmployeeID  AS NvarChar(10);
USE [database] 
    GO
    SET ANSI_NULLS ON
    GO
    SET QUOTED_IDENTIFIER ON
    GO
    Alter PROCEDURE [dbo].[spSummary]
        -- Add the parameters for the stored procedure here
    AS
    BEGIN

    DECLARE @StatusTotal AS Int
    DECLARE @Status0 AS Int
    DECLARE @Status1 AS Int
    DECLARE @Status2 AS Int
    DECLARE @Status3 AS Int

    --Cursor to loop through
    DECLARE @EmployeeID  AS NvarChar(10);


    /* DECLARE propCurs CURSOR FOR
    SELECT  EmployeeID FROM [TblTableList]

    --Open Loop Code
    OPEN propCurs;
    FETCH NEXT FROM propCurs INTO @EmployeeID
    WHILE @@FETCH_STATUS = 0  
    --Now do repeated code

      BEGIN */

     -- SELECT @StatusTotal = COUNT(ID) FROM [TblTableCountFrom] WHERE EmployeeID = @EmployeeID  
     -- SELECT @Status0 =  COUNT(DISTINCT(ID)) FROM [TblTableCountFrom] WHERE EmployeeID = @EmployeeID AND CurrentAlertStatus = '0'
     -- SELECT @Status1 =  COUNT(DISTINCT(ID)) FROM [TblTableCountFrom] WHERE EmployeeID = @EmployeeID AND CurrentAlertStatus = '1'
     -- SELECT @Status2 =  COUNT(DISTINCT(ID)) FROM [TblTableCountFrom] WHERE EmployeeID = @EmployeeID AND CurrentAlertStatus = '2'
     -- SELECT @Status3 =  COUNT(DISTINCT(ID)) FROM [TblTableCountFrom] WHERE EmployeeID = @EmployeeID AND CurrentAlertStatus = '3'

    with cnt as ( 
       SELECT 
         1 as cntAll 
         , case WHEN CurrentAlertStatus = '0' then 1 else 0  end as cnt1
         , case WHEN CurrentAlertStatus = '1' then 1 else 0  end as cnt2
         , case WHEN CurrentAlertStatus = '2' then 1 else 0  end as cnt3
         , case WHEN CurrentAlertStatus = '3' then 1 else 0  end as cnt4
       from [TblTableCountFrom] where EmployeeID = @EmployeeID
    )
    SELECT @StatusTotal = sum(cntAll),  
      @Status0  = sum(cnt1),
      @Status1  = sum(cnt2),
      @Status2  = sum(cnt3),
      @Status3  = sum(cnt4)
      FROM cnt;
        -- Check if ID Exists

        --HERE NEED TO CHECK IF EXISTS OR SHOULD I WIPE THE TABLE AT THE START? 
        IF EXISTS (SELECT EmployeeID FROM [TblTableCount] WHERE @EmployeeID = EmployeeID)

            UPDATE [TblTableCountFrom]Count 
            SET StatusTotalCount = @StatusTotal, Status0 = @Status0, Status1 = @Status1, Status2 = @Status2, Status3 = @Status3
            WHERE EmployeeID = @EmployeeID
          Else
            INSERT INTO [TblTableCount] 
            (EmployeeID, StatusTotalCount, Status0, Status1 , Status2, Status3) VALUES (@EmployeeID, @StatusTotal,@Status0, @Status1, @Status2,@Status3 )

        /* END

        -- Now we bookend the loop code

        FETCH NEXT FROM propCurs INTO @EmployeeID */

    END

    --Tidy up and close loop code
    /* CLOSE propCurs;
    DEALLOCATE propCurs; */

Context

StackExchange Code Review Q#57284, answer score: 6

Revisions (0)

No revisions yet.