Converting Dirty SQL Code to Something Clean and Efficient

Question

I have to modify some SQL code that does not seem to be working as it is supposed to.

The SQL code looks awful to me, but it works for the most part.

Say we had multiple vendors with similar names: Microsoft, Microsoft Corp, and Microsoft, Inc, etc.

All the query returns is Microsoft, even though the existing code includes the line PRI_VENDOR_NAME like '%' @PRI_VENDOR_NAME '%' (or, at least it looks like it does).

I can't seem to check to see if the code is working because it is one big, nasty looking piece of code that is appending data to a long string to execute.

CURRENT PROCEDURE: (Get ready to scream)

ALTER PROCEDURE [dbo].[GetSignalMasterByFilter]
(
@planner varchar(50),
@reorder int,
@release int,
@CMTTED varchar(50),
@partid varchar(50),
@global_short_dt int,
@PRI_VENDOR_NAME varchar(50)
)
AS
BEGIN

DECLARE @Filter nvarchar(4000)
set @Filter = ' '

if @planner <> ''
begin
set @Filter = ' and planner in('  +  @planner + ')'
end

if @reorder = 1
begin
set @Filter = rtrim(@Filter) +  ' and (REORDER_50 = ' +  char(39) + 'Y' + char(39) +  ' )  '
end

if @reorder = 2
begin
set @Filter = rtrim(@Filter) +  ' and (REORDER_30 = ' +  char(39) + 'Y' + char(39) +  ' )  '
end

if @reorder = 3
begin
set @Filter = rtrim(@Filter) +  ' and (REORDER_POINT = ' +  char(39) + 'Y' + char(39) +  ' )  '
end

--if @noaction = 1
--begin
--set @Filter = rtrim(@Filter) +  ' and reorder in (' +  char(39) + 'Excess'  + char(39) + ',' + char(39) + 'Watch'  + char(39)  + ')'
--end

if @release = 1
begin
set @Filter = rtrim(@Filter) +  ' and (RELEASE_50 = ' +  char(39) + 'Y' + char(39) +  ' )  '
end

if @release = 2
begin
set @Filter = rtrim(@Filter) +  ' and (RELEASE_30 = ' +  char(39) + 'Y' + char(39) +  ' )  '
end

if @release = 3
begin
set @Filter = rtrim(@Filter) +  ' and (RELEASE_POINT = ' +  char(39) + 'Y' + char(39) +  ' )  '
end

if @CMTTED <> 'View ALL'
begin
set @Filter = rtrim(@Filter) +  ' and CMTTED > ' +  char(39) + '0'  + char(39)  + ' and isnumeric(CMTTED) = 1 '
end

if @global_short_dt = 1
begin
set @Filter = rtrim(@Filter) +  ' and  (global_short_dt is not null or cast(CMTTED as int) >  cast(ON_HAND as int)) ' 
end

if @global_short_dt = 2 
begin
set @Filter = rtrim(@Filter) +  ' and  (global_short_dt is not null or cast(CMTTED as int) >  cast(ON_HAND as int)) AND ((cast(QTY_IN_STATUS as float) + cast(ON_ORDER as float) + cast(ON_HAND as float)) < cast(CMTTED as int)) ' 
end

if @partid <> ''
begin
set @Filter = rtrim(@Filter) +  ' and partid like(' +  char(39) + @partid + '%' +  char(39) + ')'
end

if @PRI_VENDOR_NAME <> ''
begin
set @Filter = rtrim(@Filter) +  ' and PRI_VENDOR_NAME like(' +  char(39) + @PRI_VENDOR_NAME + '%' +  char(39) + ')'
end



DECLARE @sql nvarchar(4000)
SET @sql  = '
SELECT DISTINCT PRIMARY_VENDOR,case when PRI_VENDOR_NAME is null then PRIMARY_VENDOR else  PRIMARY_VENDOR +'   +  char(39) +    ' - '  +  char(39) +  '+ PRI_VENDOR_NAME end as PRI_VENDOR_NAME 
FROM SignalReportView WHERE PRIMARY_VENDOR is not null '  + rtrim(@filter)  + ' order by PRI_VENDOR_NAME'
--print @sql
EXEC sp_executesql @sql

end

What I want to do is replace that nasty looking string variable with something that I've started below, but SQL is not my strength so it isn't quite returning any data just yet:

MY PROCEDURE VERSION: Does not return the data, but appears to be cleaner and easier to maintain in the future.

ALTER PROCEDURE GetSignalMasterByFilter2(
  @planner varchar(50),
  @reorder int,
  @release int,
  @CMTTED varchar(50),
  @partid varchar(50),
  @global_short_dt int,
  @PRI_VENDOR_NAME varchar(50)
) as begin

  SELECT DISTINCT
    PRIMARY_VENDOR,
    case when PRI_VENDOR_NAME is null then PRIMARY_VENDOR else PRIMARY_VENDOR +' - '+ PRI_VENDOR_NAME end as PRI_VENDOR_NAME 
  FROM
    SignalReportView
  WHERE
    (PRIMARY_VENDOR is not null)
    and (
      ISNULL(@planner,0)=0 or
      planner in (@planner))
    and (
      (@reorder=1 and REORDER_50='Y') or
      (@reorder=2 and REORDER_30='Y') or 
      (@reorder=3 and REORDER_POINT='Y') or 
      (1=1)
    )
    and (
      (@release=1 and RELEASE_50='Y') or
      (@release=2 and RELEASE_30='Y') or
      (@release=3 and RELEASE_POINT='Y') or
      (1=1)
    )
    and (
      (@CMTTED='View ALL') or
      (0<CMTTED and ISNUMERIC(CMTTED)=1)
    )
    and (
      (
        (@global_short_dt=1) and
        (
          (GLOBAL_SHORT_DT is not null) or
          (CAST(ON_HAND as int) < CAST(CMTTED as int))
        )
      ) or
      (1=1)
    )
    and (
      (
        (@global_short_dt=2) and
        (
          (GLOBAL_SHORT_DT is not null) or
          (
            (CAST(ON_HAND as int) < CAST(CMTTED as int)) and 
            ((CAST(QTY_IN_STATUS as float) + CAST(ON_ORDER as float) + CAST(ON_HAND as float)) < CAST(CMTTED as int))
          )
        )
      ) or
      (1=1)
    )
    and (
      ISNULL(@partid,0)=0 or
      (PARTID like '%'+@partid+'%')
    )
    and (
      ISNULL(@PRI_VENDOR_NAME,0)=0 or
      (PRI_VENDOR_NAME like '%'+@PRI_VENDOR_NAME+'%')
    )
  ORDER BY PRI_VENDOR_NAME

end

So, my question is:

Is it a good idea to rewrite the original script with a version that should be easier for other developers to maintain in the future?

If NO, can someone spot why the existing SQL is not returning all vendors?

If YES, can someone guide me with the design of my version? It is NOT currently working - probably because I have some logic wrong. Also, the (1=1) clauses do not set well with me, but I don't know a way around them. Since my procedure does not return any data, I can not use it at this point.

I apologize for not posting the table structures, but they are all rather large, and the Stored Procedure above queries an even nastier looking view (that I can't even follow).

Answer 1

Try something like this:

ALTER PROCEDURE GetSignalMasterByFilter2(
  @planner varchar(50),
  @reorder int,
  @release int,
  @CMTTED varchar(50),
  @partid varchar(50),
  @global_short_dt int,
  @PRI_VENDOR_NAME varchar(50)
) as 

begin

  SELECT DISTINCT
    PRIMARY_VENDOR,
    case when PRI_VENDOR_NAME is null then PRIMARY_VENDOR else PRIMARY_VENDOR +' - '+ PRI_VENDOR_NAME end as PRI_VENDOR_NAME 
  FROM
    SignalReportView
  WHERE
       PRIMARY_VENDOR is not null
    and 
       (
           @Planner IS NULL 
        OR @planner = ''
        OR planner in (@planner))
    and 
    (  @reorder NOT IN (1,2,3) OR
        (@reorder=1 and REORDER_50='Y') or
        (@reorder=2 and REORDER_30='Y') or 
        (@reorder=3 and REORDER_POINT='Y') 
    )
    and 
    (
       @release NOT IN (1,2,3) OR
      (@release=1 and RELEASE_50='Y') or
      (@release=2 and RELEASE_30='Y') or
      (@release=3 and RELEASE_POINT='Y') 
    )
    and 
    (
      @CMTTED='View ALL' or
      0<CMTTED and ISNUMERIC(CMTTED)=1
    )
    and 
    ( 
       @global_short_dt NOT IN (1,2) OR
       (global_short_dt is not NULL AND @global_short_dt=1 AND CAST(ON_HAND as int) < CAST(CMTTED as int)) OR
       (global_short_dt is not NULL AND @global_short_dt=2 AND CAST(ON_HAND as int) < CAST(CMTTED as int) 
           and (CAST(QTY_IN_STATUS as float) + CAST(ON_ORDER as float) + CAST(ON_HAND as float)) < CAST(CMTTED as int))
    )
    and 
    (
      @partid IS NULL OR 
      @partid = '' OR
      PARTID like '%'+@partid+'%'
    )
    and 
    (
      @PRI_VENDOR_NAME IS NULL OR
      @PRI_VENDOR_NAME = '' OR
      PRI_VENDOR_NAME like '%'+@PRI_VENDOR_NAME+'%'
    )
  ORDER BY PRI_VENDOR_NAME

end

I think I have fixed all your logic mistakes, but as I don't have any tables it is not tested.

As for the performance, you'd have to check both versions and see. There is no guarantee either way.

Answer 2

I think G Mastros gave you the right answer for the question. I only want to clarify as I see it myself.

First of all if your procedure will execute faster, nothing else matters with large data. Secondly, your version is much harder to follow for me than the first one. And by adding comments to each param and set it will be totally easier to read.

So my answer is NO and of course it's not returning all vendors because of conditions in @filter. Just print it before execution to see conditions that maked up earlier by given parameters.

Answer 3

I have been in a similar situation many times. With me.... it's usually my own code that I am trying to clean up.

When doing things like this, please do not ONLY consider code readability. You must also consider the impact on the server. Often times, there are various ways to write a query that produces identical results. In those situations, you should pick the version that executes the fastest. If this means you are using the "uglier" version, so be it.

Clearly, you looked at the original code and thought, "Huh?". This is a good indication that there should be code comments.

I haven't spent much time looking at the code, but it appears as though there are various optional parameters to the procedure (option in that empty string indicates that the code should ignore that parameter). It is possible to write code that accommodates this situation without using dynamic sql, but that code almost always executes slower. Read here for an explanation: Do you use Column=@Param OR @Param IS NULL in your WHERE clause? Don't, it doesn't perform