-5
\$\begingroup\$

I have this stored procedure that I'd like improved. I have changed the DB name and the procedure name. I am working on SQL Server 2000.

 USE [DBName]

ALTER PROCEDURE [dbo].[Table_GetSomething] 
    @name nvarchar(50), 
    @Cooltree nvarchar(10),
    @aID int,
    @bID int = 0
AS
BEGIN
    -- SET NOCOUNT ON added to prevent extra result sets from
    -- interfering with SELECT statements.
    SET NOCOUNT ON;

declare @bCE bit = 0
declare @aCE bit = 0
declare @bCE2 bit = 0
declare @aCE2 bit = 0
declare @tableName varchar(60)
declare @table table
(
    FA nvarchar(255),
    FN nvarchar(255),
    [Seeboj] nvarchar(1024),
    Brotty nvarchar(4000),
    CD nvarchar(4000)
)

if( select count(*)
    from ECCB
    where aID = @aID
    and bID = @bID
    and [Name] = @name) = 1
    set @bCE = 1
else
    if( select count(*)
    from ECCA
    where aID = @aID
    and [Name] = @name) > 0
    set @aCE = 1

if( select count(*)
    from ECCDB
    where aID = @aID
    and bID = @bID) > 0
    set @bCE2 = 1
else
    if( select count(*)
    from ECCDA
    where aID = @aID) > 0
    set @aCE2 = 1


if @bCE = 1
    insert into @table (FA, FN, [Seeboj], Brotty)
        select FA, FN, [Seeboj], Brotty
        from ECCB
        where aID = @aID
        and bID = @bID
        and [Name] = @name
else
    if(@aCE = 1)
        insert into @table (FA, FN, [Seeboj], Brotty)
            select FA, FN, [Seeboj], Brotty
            from ECCA
            where aID = @aID
                and [Name] = @name
    else
        insert into @table (FA, FN, [Seeboj], Brotty)
            select FA, FN, [Seeboj], Brotty
            from EDC
            where Cooltree = @Cooltree
            and [Name] = @name

if(@bCE2 = 1)
    update @table
    set CD =
        (select CD
         from ECCDB
         where aID = @aID
         and bID = @bID)
else
    if(@aCE2 = 1)
        update @table
        set CD =
            (select CD
             from ECCDA
             where aID = @aID)
    else
        update @table
        set CD =
            (select CD
             from EDCCD
             where Cooltree = @Cooltree)

select * from @table
END
\$\endgroup\$
9
  • 7
    \$\begingroup\$ The picture is missing and an explanation what this does. \$\endgroup\$
    – Bobby
    Jan 27, 2014 at 11:19
  • 12
    \$\begingroup\$ I agree with @Bobby. Please provide sufficient context; it's really hard to review SQL not knowing what the schema looks like and what you are trying to accomplish. \$\endgroup\$ Jan 27, 2014 at 17:34
  • 3
    \$\begingroup\$ Or in different words: With no schema, people may assume you did everything there on purpose and therefor won't answer. \$\endgroup\$ Jan 27, 2014 at 17:41
  • 3
    \$\begingroup\$ More to the point: reading this code gives the distinct impression that any real problems here are not with the code, but with the underlying schema. "Normalization nightmare" is the alliterative phrase I might use. It is hard to tell when you have so thoroughly obfuscated any explanatory value in the variable names, but that's my gut reaction to this sequence of operations. \$\endgroup\$ Jan 27, 2014 at 20:46
  • 7
    \$\begingroup\$ Even more to the point, without a description of what the code is supposed to do, then the only reference is what the code actually does, as a result, the code is doing exactly what it does, thus it is perfect. \$\endgroup\$
    – rolfl
    Jan 29, 2014 at 14:30

1 Answer 1

33
\$\begingroup\$

You have a lot of if statements in your SQL. This screams at me that you are doing something in your SQL that should be done in the application, and that this SQL should be separated into two distinct stored procedures.

The benefits of doing this:

  1. Faster stored procedures
  2. Use a coding language instead of a query language to do the logic where it was meant to be done, it will be way easier to do logic in something like C# or VB.

You need to look at what you are trying to do as a whole here, because this doesn't look like much fun.


You can get rid of all of these variables

declare @bCE bit = 0
declare @aCE bit = 0
declare @bCE2 bit = 0
declare @aCE2 bit = 0
declare @tableName varchar(60)

The @tableName variable I didn't see being used anywhere.

Instead of having two sets of if then statements you can merge the actions with the checks, something like this

IF(SELECT COUNT(*)
    FROM ECCB
    WHERE aID = @aID
        AND bID = @bID
        AND [Name] = @name) = 1
BEGIN
    INSERT INTO @table (FA, FN, [Seeboj], Brotty)
    SELECT FA, FN, [Seeboj], Brotty
    FROM ECCB
    WHERE aID = @aID
        AND bID = @bID
        AND [Name] = @name
END
ELSE
BEGIN
    IF(SELECT COUNT(*)
        FROM ECCA
        WHERE aID = @aID
            AND [Name] = @name) > 0
    BEGIN
        INSERT INTO @table (FA, FN, [Seeboj], Brotty)
        SELECT FA, FN, [Seeboj], Brotty
        FROM ECCA
        WHERE aID = @aID
            AND [Name] = @name
    END
    ELSE
    BEGIN
        INSERT INTO @table (FA, FN, [Seeboj], Brotty)
        SELECT FA, FN, [Seeboj], Brotty
        FROM EDC
        WHERE Cooltree = @Cooltree
            AND [Name] = @name
    END
END


IF(SELECT COUNT(*)
    FROM ECCDB
    WHERE aID = @aID
        AND bID = @bID) > 0
BEGIN
    UPDATE @table
    SET CD =
        (SELECT CD
        FROM ECCDB
        WHERE aID = @aID
            AND bID = @bID)
END
ELSE
BEGIN
    IF(SELECT COUNT(*)
        FROM ECCDA
        WHERE aID = @aID) > 0
    BEGIN
        UPDATE @table
        SET CD =
            (SELECT CD
            FROM ECCDA
            WHERE aID = @aID)
    END
    ELSE
    BEGIN
        UPDATE @table
        SET CD =
            (SELECT CD
            FROM EDCCD
            WHERE Cooltree = @Cooltree)
    END
END

You might have noticed that I also added BEGIN and END statements for the different blocks, I think this adds to the readability of the code, it might even be necessary, I am not sure in SQL SERVER 2000 though, I normally code in 2008.

I also capitalized all the fun SQL SERVER keywords, it doesn't really matter except for readability, but I believe that it is standard practice to capitalize these words even though SQL SERVER is case insensitive when it comes to keywords.


With all the changes that I made it looks like this:

USE [DBName]

ALTER PROCEDURE [dbo].[Table_GetSomething] 
    @name NVARCHAR(50), 
    @Cooltree NVARCHAR(10),
    @aID INT,
    @bID INT = 0
AS
BEGIN
    -- SET NOCOUNT ON added to prevent extra result sets from
    -- interfering with SELECT statements.
    SET NOCOUNT ON;

    DECLARE @table TABLE
    (
        FA NVARCHAR(255),
        FN NVARCHAR(255),
        [Seeboj] NVARCHAR(1024),
        Brotty NVARCHAR(4000),
        CD NVARCHAR(4000)
    )

    IF(SELECT COUNT(*)
        FROM ECCB
        WHERE aID = @aID
            AND bID = @bID
            AND [Name] = @name) = 1
    BEGIN
        INSERT INTO @table (FA, FN, [Seeboj], Brotty)
        SELECT FA, FN, [Seeboj], Brotty
        FROM ECCB
        WHERE aID = @aID
            AND bID = @bID
            AND [Name] = @name
    END
    ELSE
    BEGIN
        IF(SELECT COUNT(*)
            FROM ECCA
            WHERE aID = @aID
                AND [Name] = @name) > 0
        BEGIN
            INSERT INTO @table (FA, FN, [Seeboj], Brotty)
            SELECT FA, FN, [Seeboj], Brotty
            FROM ECCA
            WHERE aID = @aID
                AND [Name] = @name
        END
        ELSE
        BEGIN
            INSERT INTO @table (FA, FN, [Seeboj], Brotty)
            SELECT FA, FN, [Seeboj], Brotty
            FROM EDC
            WHERE Cooltree = @Cooltree
                AND [Name] = @name
        END
    END


    IF(SELECT COUNT(*)
        FROM ECCDB
        WHERE aID = @aID
            AND bID = @bID) > 0
    BEGIN
        UPDATE @table
        SET CD =
            (SELECT CD
            FROM ECCDB
            WHERE aID = @aID
                AND bID = @bID)
    END
    ELSE
    BEGIN
        IF(SELECT COUNT(*)
            FROM ECCDA
            WHERE aID = @aID) > 0
        BEGIN
            UPDATE @table
            SET CD =
                (SELECT CD
                FROM ECCDA
                WHERE aID = @aID)
        END
        ELSE
        BEGIN
            UPDATE @table
            SET CD =
                (SELECT CD
                FROM EDCCD
                WHERE Cooltree = @Cooltree)
        END
    END
SELECT * FROM @table
END
\$\endgroup\$

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