SQL Stored Procedure Get Distinct and Update

Question

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

• RubberDuck: Can you explain why you need to store the count vs. querying the count when you need it? It would also help if we knew what flavor of SQL you're using.

Answer 1

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.

• RubberDuck: Although the semicolons aren't required, they someday will be. It's also part of the standard. I would say it's advisable to use them.
• Phrancis: Yes, good point.

Answer 2

Nitpicks

• The keyword capitalization is inconsistent. Alter PROCEDURE should be ALTER PROCEDURE. Likewise with case WHEN ... then 1 else 0 end as cntX. Keywords should be capitalized to distinguish them from variables and fields. They should at least be consistent.
• @Status0 through @Status3 are pretty bad variable names. I have no idea what the difference between the different statuses is, so I can't offer anything better unfortunately.

Not so nit-picky

• I have serious concerns about storing counts statically in a table. What happens if I add a new record? When I query the count from the field, it will lie to me. It's better to calculate the count when you actually need it. It's preferable to create a view to return this kind of data. If you absolutely insist that the count be stored in a table, an update trigger would be more appropriate.
• Generally speaking, cursors are to be avoided. Set based approaches are always better in sql.

The code in question actually looks pretty good beyond the nit-picky stuff. It's just probably not the best approach to take.

• indofraiser: Nitpick away :-) Will change them.
• indofraiser: In terms of the counting how I would love to do in on the fly but the client wants a big table showing them all in one go. In fairness in this case the stored procedure is run only when data is added/edited rather than when the viewing page is opened. I will try to add more limits to what it looks at though in the future, this was a bit 'ikky' in that it has to check everything due to it being a pushed in at the moment knowing there is a clean build to be done. :-)
• RubberDuck: Then why not create a view that counts by grouping on the different statuses? It gets calculated on the fly and it looks like a table to the client.
• indofraiser: I did but then they wanted to view A-Z, Z-A so with those changes Gridview seems easier

Answer 3

Before I even got to your code I found 2 tables that were named so badly it made me want to stop reading and post an answer.

TblTableList

TblTableStatusCount

Then I glanced at the code and found another

TblTableCount

please tell me that these are not the names of permanent tables in your Database!!!

bad hungarian notation with pascal casing that should be camel casing. ugh!

TblTableCount that is really redundant.

I don't know what these are being used for but I am sure you can think of better names than these.

---

Don't try to make your variable/table names shorter in code, EVER, it makes the code more obscure than it has to be and saving the time typing out the code now could mean ripping hair out later because you don't remember what your code is doing.

• indofraiser: No, testing. I build everything in test with generic names first then use that to build the 'real life one'. All names are given 'readable' names i.e. TblSystemUsers, TblEmployeeStatistics etc...
• Malachi: that is good, but TblTable? I normally build my queries in Test as well, but I write them readable with actual column names and table names so that I can move the query into staging, test it some more, then move it straight to production, no rewriting for production because you could add typos or whatever, just eliminate the places where something could go wrong. hazard planning.