FInd the correct Casing description from process tracking - Scalar Function

Question

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

• S3S: 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?
• Magisch: @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.

Answer 1

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

Answer 2

( 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;