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

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

-- Return the resulting string to the user
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).
  • \$\begingroup\$ Your approach to string concatenation is not guaranteed. stackoverflow.com/questions/15138593/… \$\endgroup\$ Commented Mar 12, 2017 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\$
    – S3S
    Commented Mar 13, 2017 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\$ Commented Apr 9, 2017 at 3:35

1 Answer 1


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.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.