3
\$\begingroup\$

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).
\$\endgroup\$
  • \$\begingroup\$ Your approach to string concatenation is not guaranteed. stackoverflow.com/questions/15138593/… \$\endgroup\$ – Martin Smith Mar 12 '17 at 19:16
  • \$\begingroup\$ You could change GOTO eom to SELECT @output RETURN in each of your IF blocks. If your #Result table is the same for all IF blocks then just declare it outside of all of the blocks (once). Also, I'd check to see if it exists first with if object_id('tempdb..#Result') is not null drop table #Result or you may prefer a table variable. Lastly I think a function in your application layer could handle this but it's more preference than anything. \$\endgroup\$ – scsimon Mar 13 '17 at 16:09
  • \$\begingroup\$ As an aside, using a if x then goto instead of a while can perform better than a typical while loop. I'm not sure if this is the case in more recent sql server versions but it's true in 05 and 08 at least. Also, have you tried using OPENQUERY in place of the temp table? \$\endgroup\$ – PenutReaper Apr 9 '17 at 3:35

Your Answer

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

Browse other questions tagged or ask your own question.