SQL Server Stored Procedure quality

Question

I'm beginner in programming and I have recently written quite an advanced stored procedure. Based on parameters with which I call it, it returns data and groups it, but that is not the most important part.

I want to focus not only on how it works, but also on quality of code. And here I have a question to advanced T-SQL developers, when you saw this code, what do you think about this, is it not overgrown or illegible?

USE XXX
GO

/****** Object:  StoredProcedure Script Date: 8/17/2017 7:46:23 AM ******/
SET ANSI_NULLS ON
GO
SET QUOTED_IDENTIFIER ON
GO

ALTER PROCEDURE [XXX].[XXX]
    @equipment_tag varchar(50),
    @postfix varchar(50),
    @time_from datetime2(0),
    @time_to datetime2(0)
AS
BEGIN
DECLARE @equipment_id smallint,
@equipment_level_tag varchar(100);

DECLARE @machineTable TABLE (Value varchar(50));
set @equipment_level_tag = (SELECT equipment_level_tag FROM [XXX].[XXX] where equipment_tag = @equipment_tag)
SET @equipment_id = (SELECT id FROM Equipment.Equipment WHERE tag = @equipment_tag);
If @equipment_level_tag = 'FACTORY'
    Begin
    INSERT INTO @machineTable select parent_tag
  FROM [XXX].[XXX] where equipment_tag like '%_packer' AND structure_tag = 'UTILITIES'
    End
Else If @equipment_level_tag  = 'DEPARTMENT'
    Begin
 INSERT INTO @machineTable SELECT parent_tag
  FROM  [XXX].[XXX] WHERE parent_id in (SELECT [equipment_id]     
  FROM  [XXX].[XXX] where parent_id in ((SELECT [equipment_id]     
  FROM  [XXX].[XXX] where parent_id = 2))) and equipment_tag like '%_packer' AND structure_tag = 'UTILITIES'
    End
Else If @equipment_level_tag  = 'CELL'
    Begin      
  INSERT INTO @machineTable select parent_tag
  FROM [XXX].[XXX] WHERE parent_id in (SELECT [equipment_id]     
  FROM [XXX].[XXX] where parent_id = @equipment_id) and equipment_tag like '%_packer' AND structure_tag = 'UTILITIES'
    End
Else If @equipment_level_tag  = 'WORK_CENTER'
    Begin      
  INSERT INTO @machineTable select parent_tag
  FROM [XXX].[XXX] where parent_id = @equipment_id and equipment_tag like '%_packer' AND structure_tag = 'UTILITIES'
    End
Else If @equipment_level_tag  = 'EQUIPMENT'
    Begin      
  INSERT INTO @machineTable select parent_tag
  FROM [XXX].[XXX] where equipment_id = @equipment_id; 
END

  SELECT place_id,name,CONVERT(datetime2(0), SWITCHOFFSET(CONVERT(datetimeoffset,time_from), DATENAME(TzOffset, SYSDATETIMEOFFSET()))),data FROM
       mes_machines_statistics
        WHERE name = 'OEE_'+@postfix AND time_from >= @time_from AND time_from < @time_to
        and place_id in (SELECT idx FROM _machines WHERE name like '%_packer' and line_idx in (SELECT * FROM @machineTable))

If @postfix = 'HOUR'
BEGIN
SELECT DATEADD(hh, DATEDIFF(hh, 0, CONVERT(datetime2(0), SWITCHOFFSET(CONVERT(datetimeoffset,time_from), DATENAME(TzOffset, SYSDATETIMEOFFSET())))),0) AS time_consume,
        SUM(consumption*multiplicator) as consume        
FROM [XXX].[XXX] where equipment_id = @equipment_id AND time_from >= @time_from AND time_from < @time_to 
GROUP BY DATEADD(hh, DATEDIFF(hh, 0, CONVERT(datetime2(0), SWITCHOFFSET(CONVERT(datetimeoffset,time_from), DATENAME(TzOffset, SYSDATETIMEOFFSET())))),0)                
        ORDER BY 1
        END
Else If @postfix = 'SHIFT'
BEGIN
 WITH Shift_Consumption_CTE (shift_id,shift_start,consume) 
 AS (
 select t1.shift_id ,t2.shift_start, SUM(t1.consumption*t1.multiplicator)
from [XXX].[XXX].[XXX] t1
 JOIN [XXX].[XXX].[XXX] t2 on t2.id = t1.shift_id
where equipment_id = @equipment_id AND CAST(shift_start AS DATETIME2(0)) >= @time_from
  AND CAST(shift_end AS DATETIME2(0)) <= @time_to
    GROUP BY 
  t1.shift_id,t2.shift_start
 )
SELECT CONVERT(datetime2(0), SWITCHOFFSET(CONVERT(datetimeoffset,shift_start), DATENAME(TzOffset, SYSDATETIMEOFFSET()))),consume from Shift_Consumption_CTE
END
Else If @postfix = 'DAY'
BEGIN
 WITH Daily_Consumption_CTE (prod_day_start,consume) 
 AS (
select t2.prod_day_start,SUM(t1.consumption*t1.multiplicator) 
from [XXX].[XXX].[XXX] t1
 join [XXX].[XXX].[XXX] t2 on t2.id = t1.shift_id
where equipment_id = @equipment_id AND prod_day >= CAST(@time_from AS DATE) 
  AND prod_day <= CAST(@time_to AS DATE)
  GROUP BY 
  t2.prod_day_start
 )
 SELECT CONVERT(datetime2(0), SWITCHOFFSET(CONVERT(datetimeoffset,prod_day_start), DATENAME(TzOffset, SYSDATETIMEOFFSET()))),consume from Daily_Consumption_CTE
 END
 END

There's no consistent formatting, e.g. keywords UPPER & lower case, almost no indentation. Most SQL editors can apply formatting automatically or try one of those SQL beautifiers — dnoeth
– dnoeth, Commented Aug 17, 2017 at 10:29

Peter Taylor · Accepted Answer · 2017-08-17 10:56:55Z

Let's just take

If @equipment_level_tag = 'FACTORY'
    Begin
    INSERT INTO @machineTable select parent_tag
  FROM [XXX].[XXX] where equipment_tag like '%_packer' AND structure_tag = 'UTILITIES'
    End
Else If @equipment_level_tag  = 'DEPARTMENT'
    Begin
 INSERT INTO @machineTable SELECT parent_tag
  FROM  [XXX].[XXX] WHERE parent_id in (SELECT [equipment_id]     
  FROM  [XXX].[XXX] where parent_id in ((SELECT [equipment_id]     
  FROM  [XXX].[XXX] where parent_id = 2))) and equipment_tag like '%_packer' AND structure_tag = 'UTILITIES'
    End
Else If @equipment_level_tag  = 'CELL'
    Begin      
  INSERT INTO @machineTable select parent_tag
  FROM [XXX].[XXX] WHERE parent_id in (SELECT [equipment_id]     
  FROM [XXX].[XXX] where parent_id = @equipment_id) and equipment_tag like '%_packer' AND structure_tag = 'UTILITIES'
    End
Else If @equipment_level_tag  = 'WORK_CENTER'
    Begin      
  INSERT INTO @machineTable select parent_tag
  FROM [XXX].[XXX] where parent_id = @equipment_id and equipment_tag like '%_packer' AND structure_tag = 'UTILITIES'
    End
Else If @equipment_level_tag  = 'EQUIPMENT'
    Begin      
  INSERT INTO @machineTable select parent_tag
  FROM [XXX].[XXX] where equipment_id = @equipment_id; 
END

Starting with the trivial stuff:

Replacing double spaces with single spaces (except in indentation)
Consistent indentation
Consistent capitalisation of keywords
Consistent use of ;
Splitting separate clauses

makes the structure a bit clearer. EQUIPMENT is a special case, and the rest seem to be in an illogical order, so let's structure the order to be in ascending complexity.

IF @equipment_level_tag = 'FACTORY'
BEGIN
    INSERT INTO @machineTable
    SELECT parent_tag
    FROM [XXX].[XXX]
    WHERE equipment_tag LIKE '%_packer' AND structure_tag = 'UTILITIES'
END
ELSE IF @equipment_level_tag = 'WORK_CENTER'
BEGIN
    INSERT INTO @machineTable
    SELECT parent_tag
    FROM [XXX].[XXX]
    WHERE parent_id = @equipment_id
        AND equipment_tag LIKE '%_packer' AND structure_tag = 'UTILITIES'
END
ELSE IF @equipment_level_tag = 'CELL'
BEGIN
    INSERT INTO @machineTable
    SELECT parent_tag
    FROM [XXX].[XXX]
    WHERE parent_id in (SELECT [equipment_id] FROM [XXX].[XXX] WHERE parent_id = @equipment_id)
        AND equipment_tag LIKE '%_packer' AND structure_tag = 'UTILITIES'
END
ELSE IF @equipment_level_tag = 'DEPARTMENT'
BEGIN
    INSERT INTO @machineTable
    SELECT parent_tag
    FROM [XXX].[XXX]
    WHERE parent_id in (SELECT [equipment_id] FROM [XXX].[XXX] WHERE parent_id in ((SELECT [equipment_id] FROM [XXX].[XXX] WHERE parent_id = 2)))
        AND equipment_tag LIKE '%_packer' AND structure_tag = 'UTILITIES'
END
ELSE IF @equipment_level_tag = 'EQUIPMENT'
BEGIN
    INSERT INTO @machineTable
    SELECT parent_tag
    FROM [XXX].[XXX]
    WHERE equipment_id = @equipment_id
END

At this point there are two things which look like they would benefit from being factored out:

A common table expression or view (maybe temporary) for

SELECT parent_id, parent_tag
FROM [XXX].[XXX]
WHERE equipment_tag LIKE '%_packer' AND structure_tag = 'UTILITIES'

A recursive CTE for the hierarchy with columns parent_id, ancestor_id, depth.

Then it reduces to something like

IF @equipment_level_tag = 'FACTORY'
BEGIN
    INSERT INTO @machineTable
    SELECT parent_tag
    FROM @FilteredEquipment
END
ELSE IF @equipment_level_tag = 'WORK_CENTER'
BEGIN
    INSERT INTO @machineTable
    SELECT parent_tag
    FROM @FilteredEquipment
        -- to expose the symmetry more: INNER JOIN @Hierarchy H ON FE.parent_id = H.parent_id
    WHERE parent_id = @equipment_id
        -- equivalently: H.depth = 0 AND H.ancestor_id = @equipment_id
END
ELSE IF @equipment_level_tag = 'CELL'
BEGIN
    INSERT INTO @machineTable
    SELECT parent_tag
    FROM @FilteredEquipment FE
        INNER JOIN @Hierarchy H ON FE.parent_id = H.parent_id
    WHERE H.depth = 1 AND H.ancestor_id = @equipment_id
END
ELSE IF @equipment_level_tag = 'DEPARTMENT'
BEGIN
    INSERT INTO @machineTable
    SELECT parent_tag
    FROM @FilteredEquipment
        INNER JOIN @Hierarchy H ON FE.parent_id = H.parent_id
    WHERE H.depth = 2 AND H.ancestor_id = 2
END
ELSE IF @equipment_level_tag = 'EQUIPMENT'
BEGIN
    INSERT INTO @machineTable
    SELECT parent_tag
    FROM [XXX].[XXX]
    WHERE equipment_id = @equipment_id
END

And it's far easier to spot the special case that DEPARTMENT uses 2 instead of @equipment_id and to either fix it or add a comment explaining why it's correct. If it's a bug then the comment suggests how the symmetry allows three cases to be collapsed into one.

paparazzo · Accepted Answer · 2017-08-19 11:18:03Z

That code is very hard to read.
No indent, inconsistent capitalization, and inconsistent use of [ ]. It is bad.

SELECT parent_tag    
FROM  [XXX].[XXX] WHERE parent_id in (SELECT [equipment_id]         
FROM  [XXX].[XXX] where parent_id in ((SELECT [equipment_id]         
FROM  [XXX].[XXX] where parent_id = 2))) and equipment_tag like '%_packer' AND structure_tag = 'UTILITIES'

This is easier to read but a query like that leads me to believe the data design has problems.

SELECT parent_tag    
FROM  [XXX].[XXX] 
WHERE parent_id in ( SELECT [equipment_id]         
                     FROM  [XXX].[XXX] 
                     where parent_id in ( ( SELECT [equipment_id]         
                                            FROM  [XXX].[XXX] 
                                            where parent_id = 2 
                                          )
                                        )
                   ) 
and equipment_tag like '%_packer' 
AND structure_tag = 'UTILITIES'

All the in could be replaced with join for likely better performance.

with cte as 
( SELECT parent_tag    
    FROM  [XXX].[XXX] 
   WHERE equipment_tag like '%_packer' 
     AND structure_tag = 'UTILITIES'
)

SELECT cte.parent_tag 
  FROM cte 
  JOIN [XXX].[XXX] p1 
    ON p1.equipment_id = cte.parent_id   
   AND p1.parent_id = 2

Stack Exchange Network

SQL Server Stored Procedure quality

2 Answers 2

You must log in to answer this question.

Hot Network Questions

SQL Server Stored Procedure quality

2 Answers 2

You must log in to answer this question.

Related

Hot Network Questions