Stored procedure to classify policies by sales date

Currently this is the store procedure I am working on, it is working according to some testing that was done. I feel that it could be improved though, could use some optimization, feedback or recommendations.

What is it supposed to do? For given policy id passed in, grab all coverages associated with the policy. For each coverage get units that are tied to the coverage. I need to check for 3 conditions for all units for a coverage which is based on Sales_Closing_Date. Depending on the condition update the coverage table with the flag.

```
DECLARE @Policy_ID BIGINT = 0,
@CurrCoverageID BIGINT = 0,
@Year INT = 0
DECLARE @Coverages TABLE(CID INT IDENTITY(1,1),CoverageID BIGINT NOT NULL)
DECLARE @Units TABLE(UID INT IDENTITY(1,1),CoverageID BIGINT NOT NULL,SalesClosing DATE)
DECLARE @CoveragesOut TABLE(CoverageID BIGINT NOT NULL, MultiFlag CHAR(1))

SET @Year = (SELECT p.Reins_Year FROM dbo.Policy p WHERE p.Policy_ID = @Policy_ID)

--INSERT ALL COVERAGES FOR THIS POLICY
INSERT INTO @Coverages(CoverageID)
SELECT Coverage_ID FROM dbo.Coverages c WHERE c.Policy_ID = @Policy_ID

--GO THROUGH COVERAGE AND GET THEIR SALES CLOSING DATE
WHILE (SELECT COUNT(*) FROM @Coverages) > 0
BEGIN
SET @CurrCoverageID = (SELECT TOP 1 CoverageID FROM @Coverages)
INSERT INTO @Units(CoverageID,SalesClosing)
SELECT DISTINCT
@CurrCoverageID,
CAST(pu.Sales_Closing_Date AS DATE)
FROM dbo.Policy_Units pu
WHERE pu.Coverage_ID = @CurrCoverageID
AND pu.Sales_Closing_Date IS NOT NULL

--WE SHOULD HAVE THE SALES CLOSING DATES NOW
--INSERT INTO OUTPUT TABLE NOW...
INSERT INTO @CoveragesOut(CoverageID,MultiFlag)
SELECT DISTINCT
u.CoverageID,
CASE (SELECT COUNT(*) FROM @Units)
WHEN 1 THEN
CASE
--SPRING--
WHEN u.SalesClosing BETWEEN CAST('1/1/' + CAST(@Year AS VARCHAR) AS DATE) AND CAST('4/30/' + CAST(@Year AS VARCHAR) AS DATE) OR u.SalesClos

I will try to cover several points:

1) Naming

Try to use homogeneous naming. Pascal/Camel case seem too be used most often, so stick to it. E.g. @Policy_ID should be @PolicyId, CoverageID should be CoverageId.

2) Improve readability of your declarations

Your table variables could be declared like this:

DECLARE @Coverages TABLE 
(
   CID INT IDENTITY(1,1),
   CoverageID BIGINT NOT NULL
)

DECLARE @Units TABLE
(
   UID INT IDENTITY(1,1),
   CoverageID BIGINT NOT NULL,
   SalesClosing DATE
)

DECLARE @CoveragesOut TABLE
(
   CoverageID BIGINT NOT NULL, 
   MultiFlag CHAR(1)
)

3) Put clauses on separate lines for better readability

First statements could be rewritten like this:

SET @Year = 
   (
      SELECT p.Reins_Year 
      FROM dbo.Policy p 
      WHERE p.Policy_ID = @Policy_ID
   )

--INSERT ALL COVERAGE FOR THIS POLICY
INSERT INTO @Coverages (CoverageID)
SELECT Coverage_ID 
FROM dbo.Coverages c 
WHERE c.Policy_ID = @Policy_ID

4) Defensive set

If by any chance, the select for year returns more that one year, set statement will fail. You might consider assigning with SELECT and/or using TOP 1, just to be sure it does not fail (however, it may return an incorrect value).

SET @Year = 
   (
      SELECT TOP 1 p.Reins_Year 
      FROM dbo.Policy p 
      WHERE p.Policy_ID = @Policy_ID
   )

5) Easier date arithmetic

You have a case for spring and fall that performs some ad-hoc computation, that can be simplified. Also, since @Year is computed before the cycle, all those boundaries can be precomputed. You did not specify SQL Server version, so I hope for the best (2012+):

DECLARE @StartOfYear DATE = DATEFROMPARTS(@Year, 1, 1)
 DECLARE @EndOfApril DATE = DATEFROMPARTS(@Year, 4, 30)
 DECLARE @FirstOfMay DATE = DATEFROMPARTS(@Year, 5, 1)

so, that the "Spring" CASE would be something like this (reformatted for better readability):

WHEN u.SalesClosing BETWEEN @StartOfYear AND @EndOfApril
   OR u.SalesClosing >= @StartOfYear 
   AND u.SalesClosing < @FirstOfMay
THEN 'S'

However, logic for the dates is unclear for me, even after reading operator precedence (AND takes precedence over OR and BETWEEN). It is strongly recommended to use parenthesis when using multiple operators with close precedence.

6) Set based logic

WHILE and other looping constructs are intruders in T-SQL language. They will usually behave (much) poorer than set based approaches. So, your WHILE can be converted into something this:

;WITH U_CTE AS (
    SELECT DISTINCT C.CoverageID, CAST(pu.Sales_Closing_Date AS DATE),
        COUNT(1) OVER (PARTITION BY C.CoverageId) AS Cnt
    FROM dbo.Policy_Units PU
        JOIN @Coverages C ON PU.Coverage_ID = C.CoverageID 
)
INSERT INTO @CoveragesOut (CoverageID, MultiFlag)
SELECT DISTINCT
    u.CoverageID,
    CASE 
        WHEN @Cnt = 1 THEN
            CASE 
               WHEN -- spring logic
               WHEN -- autumn logic
        WHEN 2 THEN 'B'
    ELSE NULL
FROM U_CTE

Also, @Units table is not required anymore and the whole block is slimier.

7) Proper data types

CAST(pu.Sales_Closing_Date AS DATE) suggests that Sales_Closing_Date is not a DATE (maybe a DATETIME or VARCHAR). If possible, use the most appropriate data type to avoid explicit or implicit (silently occurring in queries) casts.

8) Comments

It's a matter of taste, but I like comments written as normal text, not using CAPS

StackExchange Code Review Q#118551, answer score: 2