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

• dnoeth: 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

Answer 1

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:

1. 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'
   

2. 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.

Answer 2

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