2
\$\begingroup\$

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
\$\endgroup\$
  • \$\begingroup\$ 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 \$\endgroup\$ – dnoeth Aug 17 '17 at 10:29
3
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
1
\$\begingroup\$

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
| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.