Stored procedure to describe the reason that a particular employee is unsuitable for a particular task

Question 1

I used the following Stack Overflow questions as references for writing this code:

The idea behind my stored procedure is to give an explanation of why a particular worker is unsuitable to care for a particular patient under specific circumstances. For example, you obviously can’t assign an RN to do physical therapy visits.

I have a Disciplines table, which has a structure that’s kind of like this:

--------------------------------------------
| Discipline | Discipline ID | Other Field |
--------------------------------------------
| RN         | 1             | 2           |
--------------------------------------------
| PT         | 2             | 5           |
--------------------------------------------
| CNA        | 3             | 7           |
--------------------------------------------
| MD         | 4             | 23           |
--------------------------------------------

Obviously this is hypothetical data and I’ve modified the column names a bit (this is mostly for illustration).

What I want is to retrieve the list of Disciplines as a single string and concatenate it with an existing string. Workers might have several disciplines. (This is determined in a table that’s not shown). An example of one possible output would be The worker does not have the appropriate disciplines. They have the following discipline(s): RN, PT (if that employee is both a RN and a PT – not too likely, obviously, but let’s assume that they are).

I haven’t included the entire stored procedure, but this is pretty representative and is the main part that I’m looking for a critique of:

 -- @IsAppropriate is a bit type
 SELECT @IsAppropriate = dbo.fnCheckIfWorkerHasDisciplineForSvc(@BusinessUnitID, @WorkerID, @ServiceTypeID, GetDate(), GetDate())

IF @IsAppropriate = 0 
BEGIN
    SET @output = 'The worker does not have the appropriate disciplines. They have the following discipline(s):'

    CREATE TABLE #Result
    (
            Discipline nvarchar(max),
            IsPrimary bit,
            EffectiveDate DateTime,
            EndDate DateTime
    )

            -- Get which discipline a particular worker practices (PT, MD, RN, etc.)
    INSERT #Result EXEC spGetWorkerDiscipline @WorkerID

    DECLARE @disciplines nvarchar(max);

            -- Get them as a nice list
    SELECT @disciplines = coalesce(@disciplines + ', ', '') + Discipline from #Result

    SET @output = @output + @disciplines

    DROP TABLE #Result

            -- Skip the rest of the stored procedure
            -- See comments below - is this a terrible thing to do?
    GOTO eom
END

-- Check some other reasons that the worker might be inappropriate...

-- Return the resulting string to the user
eom:
select @output

I verified that the query does, in fact, work. However, I have a few concerns:

Is using GOTO like that in SQL really terrible? If so, is there a better way to accomplish the same thing? I’m almost embarrassed to post code that contains it, since I’m aware that, in general, GOTO is considered harmful.
Will I take a performance hit for creating and dropping a temporary table like that? Could some of the other suggestions in the linked posts be a better choice here?
Am I inappropriately mixing my architectural layers by creating a string for use on the front end in the database like this? (I’m using ASP.NET Web Forms).

Question 2

A handful of critiques:

As mentioned in the comments, this is undefined behavior: SELECT @val = @val + ColumnName FROM AnyTable. As such, you should avoid it. If you need to, use STUFF or STRING_AGG instead
When creating a temp table, (n)varchar columns should specify their collation (e.g. COLLATE DATABASE_DEFAULT ) to avoid errors in case server collation and database collation differ.
Using RETURN or a chain of IF ELSE IF etc is going to raise fewer eyebrows than GOTO
Don’t just blindly insert into a table; always specify a column list.
Its cheaper to use table variables than temp tables; SQL Server doesn’t need to do as much work for statistics and fewer latches of creation are required.
You generally don’t need to drop the temp table (assuming this is called from a stored procedure) as that’ll get done automatically by SQL Server
You could probably just return the list of disciplines + an error code, and let the consuming application handle presenting it in a friendly way. This also makes I18N someday in the future much easier.

Leave a Reply Cancel reply