3
\$\begingroup\$

It works as intended, I just wonder if there is any stylistic things I could improve. It already runs well fast enough, although I'd be open to speed increases.

This SQL Server 2008 Scalar Function takes a tracking id from our internal system and finds the corresponding casing type for the products being processed. It distinguishes between where the products currently are in processing and will attempt to find casings for the first unresolved processing step first.

Some notes:

  • Variable names are translated from german
  • underscore case is used for variable names

Here is the code:

SET ANSI_NULLS ON
GO
SET QUOTED_IDENTIFIER ON
GO

CREATE FUNCTION dbo.GetCasingFromTrackingId 
(
    @tracking_id integer
)
RETURNS nvarchar(200)
AS
BEGIN
    DECLARE @result nvarchar(200);
    DECLARE @intermediate_result nvarchar(200);
    DECLARE @first_procstep_id integer;
    DECLARE @steps_ort_id integer;
    DECLARE @procstep_config_id integer;
    DECLARE @current_procstep_id integer;
    DECLARE @current_procstep_ort_id integer;
    DECLARE @current_procstep_config_id integer;
    DECLARE @current_procstep_file_id integer;
    DECLARE @loop_counter integer;
    DECLARE @step_count integer;

    SELECT TOP 1 
    @first_procstep_id = ar.Eintrag, @steps_ort_id = vg.OrtID, @procstep_config_id = ar.FileID
    FROM [CompanyDB].[dbo].[ProcSteps] AS ar 
    INNER JOIN [CompanyDB].[dbo].[Workflow] AS vg ON ar.VorgangID = vg.Eintrag
    WHERE ( vg.OrtID <> 0 ) AND ( vg.Name NOT LIKE '%entgur%' ) AND ( ar.AuftragID = @tracking_id )
    ORDER BY ar.Position;

    IF ( @first_procstep_id IS NOT NULL ) AND ( @procstep_config_id IS NOT NULL )
        BEGIN
            IF @steps_ort_id = 1
                BEGIN
                    SET @intermediate_result =
                    ( 
                    SELECT DISTINCT TOP 1
                    c.Gehaeuse
                    FROM [CompanyDB].[dbo].[Config] AS c
                    INNER JOIN [CompanyDB].[dbo].[ProcSteps] AS ar ON ar.Config_ID = c.Eintrag
                    INNER JOIN [CompanyDB].[dbo].[ProcTracking] AS lz on ar.AuftragID = lz.Eintrag
                    WHERE ( c.Gehaeuse IS NOT NULL ) AND ( lz.Eintrag = @tracking_id )
                    ORDER BY c.Gehaeuse
                    );
                    IF @intermediate_result IS NOT NULL
                            SET @result = @intermediate_result;
                    ELSE
                            SET @result = 'n.a.'    
                END
            ELSE IF @steps_ort_id = 2
                BEGIN
                    SET @intermediate_result =
                    (
                    SELECT TOP 1
                    tpl.[Gehäuse]
                    FROM [CompanyDB].[dbo].[Filelist] AS tpl
                    WHERE ( tpl.Eintrag = @procstep_config_id )
                    );
                    IF @intermediate_result IS NOT NULL
                            SET @result = @intermediate_result;
                    ELSE
                            SET @result = 'n.a.';
                END
            ELSE IF @steps_ort_id = 3
                BEGIN
                    SET @intermediate_result =
                    (
                    SELECT TOP 1
                    gu.[Gehäuse]
                    FROM [CompanyDB].[dbo].[Bands] AS gu
                    WHERE gu.Eintrag = @procstep_config_id
                    ORDER BY gu.Eintrag
                    );
                    IF @intermediate_result IS NOT NULL
                            SET @result = @intermediate_result;
                    ELSE
                            SET @result = 'n.a.'
                END
            ELSE IF @steps_ort_id = 4
                BEGIN
                    SET @result = 'n.a.';
                END
            ELSE IF @steps_ort_id = 5
                BEGIN
                    SET @result = 'n.a.';
                END
            ELSE IF @steps_ort_id = 7
                BEGIN
                    SET @result = 'n.a.';
                END
            ELSE
                BEGIN
                    SET @loop_counter = 1;
                    SET @step_count = 
                    (
                    SELECT
                    COUNT(ar.Eintrag)
                    FROM [CompanyDB].[dbo].[ProcSteps] AS ar 
                    INNER JOIN [CompanyDB].[dbo].[Workflow] AS vg ON ar.VorgangID = vg.Eintrag
                    WHERE ( vg.OrtID <> 0 ) AND ( vg.Name NOT LIKE '%entgur%' ) AND ( ar.AuftragID = @tracking_id )
                    );
                    WHILE @loop_counter <= @step_count
                        BEGIN
                            SET @current_procstep_config_id = NULL;
                            SET @current_procstep_file_id = NULL;
                            SET @current_procstep_id = NULL;
                            SET @current_procstep_ort_id = NULL;

                            SELECT @current_procstep_id = lzschritte.Eintrag, @current_procstep_ort_id = lzschritte.Ort, @current_procstep_config_id = lzschritte.Config FROM 
                                (
                                SELECT 
                                ar.Eintrag AS Eintrag,
                                vg.OrtID AS Ort,
                                ar.Config_ID AS Config,
                                ROW_NUMBER() OVER (ORDER BY ar.Eintrag) AS current_row
                                FROM [CompanyDB].[dbo].[ProcSteps] AS ar
                                INNER JOIN [CompanyDB].[dbo].[Workflow] AS vg ON ar.VorgangID = vg.Eintrag
                                WHERE ( vg.OrtID <> 0 ) AND ( vg.Name NOT LIKE '%entgur%' ) AND ( ar.AuftragID = @tracking_id )
                                ) AS lzschritte
                            WHERE ( lzschritte.current_row = @loop_counter );

                            IF @current_procstep_ort_id IS NOT NULL
                                BEGIN
                                    IF @current_procstep_ort_id = 1
                                        BEGIN
                                            IF @current_procstep_config_id IS NOT NULL
                                                BEGIN
                                                    IF ( SELECT c.Geraet2 FROM [CompanyDB].[dbo].[Config] AS c WHERE c.Eintrag = @current_procstep_config_id ) IS NOT NULL
                                                        BEGIN
                                                            SET @intermediate_result = ( SELECT c.Gehaeuse FROM [CompanyDB].[dbo].[Config] AS c WHERE c.Eintrag = @current_procstep_config_id);
                                                            IF @intermediate_result IS NOT NULL
                                                                SET @result = @intermediate_result;
                                                            ELSE
                                                                SET @result = 'n.a.';
                                                            BREAK;
                                                        END
                                                END
                                        END
                                    ELSE IF @current_procstep_ort_id = 2
                                        BEGIN
                                            SET @current_procstep_file_id = ( SELECT ar.FileID FROM [CompanyDB].[dbo].[ProcSteps] AS ar WHERE ar.Eintrag = @current_procstep_id );
                                            IF @current_procstep_file_id IS NOT NULL    
                                                BEGIN
                                                    SET @intermediate_result = ( SELECT tpl.[Gehäuse] FROM [CompanyDB].[dbo].[Filelist] AS tpl WHERE tpl.Eintrag = @current_procstep_file_id);
                                                    IF @intermediate_result IS NOT NULL
                                                        SET @result = @intermediate_result;
                                                    ELSE
                                                        SET @result = 'n.a.';
                                                    BREAK;
                                                END     
                                        END
                                    ELSE IF @current_procstep_ort_id = 3
                                        BEGIN
                                            SET @current_procstep_file_id = ( SELECT ar.FileID FROM [CompanyDB].[dbo].[ProcSteps] AS ar WHERE ar.Eintrag = @current_procstep_id );
                                            IF @current_procstep_file_id IS NOT NULL    
                                                BEGIN
                                                    SET @intermediate_result = ( SELECT gu.[Gehäuse] FROM [CompanyDB].[dbo].[Bands] AS gu WHERE gu.Eintrag = @current_procstep_file_id);
                                                    IF @intermediate_result IS NOT NULL
                                                        SET @result = @intermediate_result;
                                                    ELSE
                                                        SET @result = 'n.a.';
                                                    BREAK;
                                                END
                                        END
                                    ELSE IF @current_procstep_ort_id = 4
                                        BEGIN
                                            SET @result = 'n.a.';
                                            BREAK;
                                        END
                                    ELSE IF @current_procstep_ort_id = 8
                                        BEGIN
                                            SET @result = 'n.a.';
                                            BREAK;
                                        END
                                END
                            SET @loop_counter = @loop_counter + 1;
                        END
                END
        END 
    ELSE
        BEGIN
            SET @result = 'n.a.';
        END                         
    RETURN @result;
END
GO
\$\endgroup\$
  • \$\begingroup\$ I'm not sure if anyone will be able to really improve this without a sample date set. I'm sure you are aware that loops are typically bad, and while there are some cursory things I could suggest there isn't any guarantee they'd improve anything. Could you provide some sample data for a specific test case? \$\endgroup\$ – scsimon Jul 11 '17 at 13:35
  • \$\begingroup\$ @scsimon I'm not sure this is possible since the data sturcture is very complicated and this is working on live company data. But I'm more looking for a syntactic / semantic critique. As I stated, it already runs well enough. \$\endgroup\$ – Magisch Jul 11 '17 at 13:54
2
\$\begingroup\$

As mentioned by @scsimon you should avoid loops in SQL as much as possible, in fact try to fire as less select statements as possible. In below rewrite I've accomplished the same result (AFAIK, test it well) with only 3 select statements while you were using between 12 and x depending on number of iteration. I'm sure giving more time you could rewrite it into a single statement and put in in a inline function instead of a scalar function.

To answer the actual question, for code formatting in a nutshell I use the following rules

  • Avoid the need for horizontal scrolling at all costs
  • Put every subsection on a new line so the reads from top to bottom, no need to read the whole line to check for additional expressions.
  • Indent when line can be considered child of previous line.
  • Align stuff both vertical and horizontal to allow your eye to find parts of the query faster

Having said that, this is the result:

CREATE FUNCTION dbo.GetCasingFromTrackingId(
      @tracking_id INTEGER
)
RETURNS NVARCHAR(200) AS
BEGIN
    DECLARE @result                     NVARCHAR(200);
    DECLARE @intermediate_result        NVARCHAR(200);
    DECLARE @check_result               NVARCHAR(200);
    DECLARE @first_procstep_id          INTEGER;
    DECLARE @steps_ort_id               INTEGER;
    DECLARE @procstep_config_id         INTEGER;
    DECLARE @current_procstep_id        INTEGER;
    DECLARE @current_procstep_ort_id    INTEGER;
    DECLARE @current_procstep_config_id INTEGER;
    DECLARE @current_procstep_file_id   INTEGER;
    DECLARE @loop_counter               INTEGER;
    DECLARE @step_count                 INTEGER;

    SELECT TOP 1 
          @first_procstep_id    = ar.Eintrag
        , @steps_ort_id         = vg.OrtID
        , @procstep_config_id   = ar.FileID
    FROM CompanyDB.dbo.ProcSteps AS ar 
        INNER JOIN CompanyDB.dbo.Workflow AS vg 
            ON ar.VorgangID = vg.Eintrag
    WHERE vg.OrtID          <> 0
        AND vg.Name         NOT LIKE '%entgur%' 
        AND ar.AuftragID    = @tracking_id
    ORDER BY ar.Position;

    IF @first_procstep_id IS NOT NULL
        AND @procstep_config_id IS NOT NULL
    BEGIN
        IF @steps_ort_id BETWEEN 1 AND 7
            SELECT @result = COALESCE((
                SELECT TOP 1 c.Gehaeuse
                FROM CompanyDB.dbo.Config AS c
                    INNER JOIN CompanyDB.dbo.ProcSteps AS ar 
                        ON ar.Config_ID = c.Eintrag
                    INNER JOIN CompanyDB.dbo.ProcTracking AS lz 
                        ON ar.AuftragID = lz.Eintrag
                WHERE c.Gehaeuse        IS NOT NULL
                    AND lz.Eintrag      = @tracking_id
                    AND @steps_ort_id   = 1
                ORDER BY c.Gehaeuse

                UNION ALL                

                SELECT TOP 1 tpl.Gehäuse
                FROM CompanyDB.dbo.Filelist AS tpl
                WHERE tpl.Eintrag       = @procstep_config_id
                    AND @steps_ort_id   = 2

                UNION ALL

                SELECT TOP 1 gu.Gehäuse
                FROM CompanyDB.dbo.Bands AS gu
                WHERE gu.Eintrag        = @procstep_config_id
                    AND @steps_ort_id   = 3
                ORDER BY gu.Eintrag
            ), 'n.a.');
        ELSE
        BEGIN
            WITH lzschritte AS(
                SELECT 
                      Eintrag     = ar.Eintrag
                    , Ort         = vg.OrtID
                    , Config      = ar.Config_ID
                    , rn          = ROW_NUMBER() OVER (ORDER BY ar.Eintrag)
                FROM CompanyDB.dbo.ProcSteps AS ar
                    INNER JOIN CompanyDB.dbo.Workflow AS vg 
                        ON ar.VorgangID = vg.Eintrag
                WHERE vg.OrtID       <> 0 
                    AND vg.Name      NOT LIKE '%entgur%'
                    AND ar.AuftragID = @tracking_id
            )
            SELECT @result = COALESCE((
                SELECT TOP 1
                      ort.result
                FROM lzschritte AS l
                    CROSS APPLY(
                        SELECT 
                              result = c.Gehaeuse 
                        FROM CompanyDB.dbo.Config AS c 
                        WHERE l.Ort = 1
                            AND c.Eintrag = l.Eintrag
                            AND c.Geraet2 IS NOT NULL

                        UNION ALL                        

                        SELECT result = tpl.Gehäuse 
                        FROM CompanyDB.dbo.ProcSteps AS ar
                            INNER JOIN CompanyDB.dbo.Filelist AS tpl 
                                ON ar.FileID = tpl.Eintrag
                        WHERE l.Ort = 2
                            AND ar.Eintrag = l.Eintrag

                        UNION ALL                        

                        SELECT result = tpl.Gehäuse 
                        FROM CompanyDB.dbo.ProcSteps AS ar
                            INNER JOIN CompanyDB.dbo.Bands AS tpl 
                                ON ar.FileID = tpl.Eintrag
                        WHERE l.Ort = 3
                            AND ar.Eintrag = l.Eintrag    

                        UNION ALL

                        SELECT result = 'n.a.'
                        WHERE l.Ort IN (4, 8)
                    ) AS ort
                ORDER BY l.rn             
            ), 'n.a.');
        END
    END
    ELSE
        SET @result = 'n.a.';       
    RETURN @result;
END
GO

Note: I haven't considered naming convention

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

( vg.OrtID <> 0 ) does not require ( )

SELECT DISTINCT TOP 1 is redundant

seems like you could have a table to map this stuff

ELSE IF @current_procstep_ort_id = 4
                                    BEGIN
                                        SET @result = 'n.a.';
                                        BREAK;
|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.