SQL Server Stored Procedure quality

Question 1

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 XXXGO/****** Object:  StoredProcedure Script Date: 8/17/2017 7:46:23 AM ******/SET ANSI_NULLS ONGOSET QUOTED_IDENTIFIER ONGOALTER PROCEDURE [XXX].[XXX]    @equipment_tag varchar(50),    @postfix varchar(50),    @time_from datetime2(0),    @time_to datetime2(0)ASBEGINDECLARE @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'    EndElse 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'    EndElse 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'    EndElse 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'    EndElse 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'BEGINSELECT 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        ENDElse 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_idwhere 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_CTEENDElse 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_idwhere 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

Question 2

There's no consistent formatting, e.g. keywordsUPPER &lower case, almost no indentation. Most SQL editors can apply formatting automatically or try one of thoseSQL beautifiers

Question 3

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'    EndElse 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'    EndElse 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'    EndElse 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'    EndElse 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'ENDELSE 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'ENDELSE 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'ENDELSE 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'ENDELSE IF @equipment_level_tag = 'EQUIPMENT'BEGIN    INSERT INTO @machineTable    SELECT parent_tag    FROM [XXX].[XXX]    WHERE equipment_id = @equipment_idEND

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_tagFROM [XXX].[XXX]WHERE equipment_tag LIKE '%_packer' AND structure_tag = 'UTILITIES'

A recursive CTE for the hierarchy with columnsparent_id,ancestor_id,depth.

Then it reduces to something like

IF @equipment_level_tag = 'FACTORY'BEGIN    INSERT INTO @machineTable    SELECT parent_tag    FROM @FilteredEquipmentENDELSE 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_idENDELSE 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_idENDELSE 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 = 2ENDELSE IF @equipment_level_tag = 'EQUIPMENT'BEGIN    INSERT INTO @machineTable    SELECT parent_tag    FROM [XXX].[XXX]    WHERE equipment_id = @equipment_idEND

And it's far easier to spot the special case thatDEPARTMENT uses2 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.

Question 4

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

Peter Taylor 24.5k1 gold badge49 silver badges94 bronze badges · 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'    EndElse 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'    EndElse 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'    EndElse 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'    EndElse 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'ENDELSE 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'ENDELSE 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'ENDELSE 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'ENDELSE IF @equipment_level_tag = 'EQUIPMENT'BEGIN    INSERT INTO @machineTable    SELECT parent_tag    FROM [XXX].[XXX]    WHERE equipment_id = @equipment_idEND

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_tagFROM [XXX].[XXX]WHERE equipment_tag LIKE '%_packer' AND structure_tag = 'UTILITIES'

A recursive CTE for the hierarchy with columnsparent_id,ancestor_id,depth.

Then it reduces to something like

IF @equipment_level_tag = 'FACTORY'BEGIN    INSERT INTO @machineTable    SELECT parent_tag    FROM @FilteredEquipmentENDELSE 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_idENDELSE 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_idENDELSE 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 = 2ENDELSE IF @equipment_level_tag = 'EQUIPMENT'BEGIN    INSERT INTO @machineTable    SELECT parent_tag    FROM [XXX].[XXX]    WHERE equipment_id = @equipment_idEND

And it's far easier to spot the special case thatDEPARTMENT uses2 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.

Movatterモバイル変換

Stack Exchange Network

SQL Server Stored Procedure quality

2 Answers2

You mustlog in to answer this question.

Related

Hot Network Questions

Subscribe to RSS