He knows about cursors, and he's not afraid to use them.



  • This post should be accompanied by a little background story. I work for a relatively small company where we use a custom developed .Net application. This application is not yet finished, so regularly, I have to install a new test version. The install often includes running a few T-SQL scripts that add some new fields, procedures and/or functions to the database. Many a time did I consider posting here... Sometimes several times a day.

    From the unfortunate, but never corrected misspellings of table and field names, via integer foreign key values stored in CHARs, to the use of many, many ugly and unneeded subqueries (I have seen update statements that have up to 20 of them), these issues were never quite nasty enough for me to actually go ahead and register. Five minutes ago, I decided it was time when I ran into the UDF you see below, which returns 0 if a client exists in the Clients table and 1 if it doesn't. I'm sure this is the first post of a few, and this is a gem that I didn't want to keep from you. To protect the guilty, I've obfuscated names (and only names!). Got cursor?

    CREATE FUNCTION dbo.IsClientNew (@ClientTypeID INT, @ClientNumber AS VARCHAR(50)) 
    RETURNS INT
    AS 
    BEGIN

    DECLARE @Client AS INT

    DECLARE ClientCursor CURSOR FOR
    SELECT dbo.Client.ClientID FROM dbo.Client
    WHERE dbo.Client.ClientTypeID = @ClientTypeID AND dbo.Client.ClientNumber = @ClientNumber

    OPEN ClientCursor

    FETCH NEXT FROM ClientCursor
    INTO @Client

    CLOSE ClientCursor
    DEALLOCATE ClientCursor

    IF @Client <> NULL RETURN 0

    RETURN 1
     
    END

    Ps. I've given up on the formatting. Somebody give me a clue as to how I'm supposed to do that please, for my next post.



  • Looks like the coder is a bit too used to working with variables instead of a much simpler IF EXISTS clause. My guess is he's moved into SQL from whatever his first programming language is and doesn't yet understand that not all languages are the same.



  • That's just wrong... a REAL programmer gets all the results and checks for a condition in the application layer.

    Note for the ever present idiots: that was sarcasm 



  • If only that was the case. See below, exhibit B (again obfuscated). This procedure has been created by the same person, well before the above UDF.

        CREATE PROCEDURE dbo.GetProductType
        @ProducerID INT,
        @CheckValue INT = 0,
        @WhatToReturn VARCHAR(10),
        @ReturnValue INT = 0 OUTPUT
        AS

        SET NOCOUNT ON
        SET  @ReturnValue = 0

        -- Yes, it's returning one of two things depending on this variable.
        -- It either selects a 'typegroup', of which a producer can only have one...
        -- Or it selects the type based on some minvalue and maxvalue.

        -- Note the peculiar use of double quotes instead of single quotes.
        IF @WhatToReturn = "TYPEGROUP"
        BEGIN
        DECLARE TypeCursor CURSOR FOR
        SELECT TypeGroupID FROM ProductType
        WHERE ProducerID = @ProducerID

        OPEN TypeCursor
        FETCH NEXT FROM TypeCursor
        INTO @ReturnValue

        CLOSE TypeCursor
        DEALLOCATE TypeCursor
        END
        ELSE    
        BEGIN

        -- Yeah, usage of the function EXISTS!
        IF  EXISTS(SELECT TypeID FROM ProductType WHERE ProducerID = @ProducerID AND MinValue <= @CheckValue AND MaxValue >= @CheckValue)
        SET @ReturnValue = (SELECT TypeID FROM ProductType WHERE ProducerID = @ProducerID AND MinValue <= @CheckValue AND MaxValue >= @CheckValue)
        ELSE
        SET @ReturnValue=0
        END

        RETURN @ReturnValue
        GO



  • I'm guessing he once read somewhere that cursors were fast. Fast is the buzzword!

    Try modifying the procedure and showing it to him. Maybe he'll accept it and learn from it.

     



  • Way to completely fail on the usage of IF EXISTS! Running the same query twice!

    Someone needs to teach that guy SELECT INTO...

    Though if it were me, I'd tell him to stop being an idiot and do this very trivial SQL code in the application itself.
     



  • Hey, at least he didn't use a cursor...



  • How about this version? My assumption is that ClientID values are unique in the Client table.

    CREATE FUNCTION dbo.IsClientNew (@ClientTypeID INT, @ClientNumber AS VARCHAR(50)) 
    RETURNS INT
    AS 
    BEGIN

    SELECT count( dbo.Client.ClientID )
    FROM dbo.Client
    WHERE dbo.Client.ClientTypeID = @ClientTypeID AND dbo.Client.ClientNumber = @ClientNumber

    END 



  • My function has the output backwards, here is the corrected version.

    CREATE FUNCTION dbo.IsClientNew (@ClientTypeID INT, @ClientNumber AS VARCHAR(50)) 
    RETURNS INT
    AS 
    BEGIN

    SELECT (1 - count( dbo.Client.ClientID )) AS Exists
    FROM dbo.Client
    WHERE dbo.Client.ClientTypeID = @ClientTypeID AND dbo.Client.ClientNumber = @ClientNumber

    END


  • @DOA said:

    That's just wrong... a REAL programmer gets all the results and checks for a condition in the application layer.

    Note for the ever present idiots: that was sarcasm 

     

    You mean, that's not the way that you're supposed to do things?

     How else would you limit your results from SQL when it only supports SELECT * FROM <database>?
     



  • @Welbog said:

    Looks like the coder is a bit too used to working with variables instead of a much simpler IF EXISTS clause. My guess is he's moved into SQL from whatever his first programming language is and doesn't yet understand that not all languages are the same.

     

    Heck, even a plain old standard 'select count(*)' would do! Considering that's one of the first things you learn when using SQL, I don't know what this guy is smoking!




  • @dmitriy said:

    My function has the output backwards, here is the corrected version.

    CREATE FUNCTION dbo.IsClientNew (@ClientTypeID INT, @ClientNumber AS VARCHAR(50)) 
    RETURNS INT
    AS 
    BEGIN

    SELECT (1 - count( dbo.Client.ClientID )) AS Exists
    FROM dbo.Client
    WHERE dbo.Client.ClientTypeID = @ClientTypeID AND dbo.Client.ClientNumber = @ClientNumber

    END

    Yes, ClientID is unique for a give type and number. How about this:

    CREATE FUNCTION dbo.IsClientNew (@ClientTypeID INT, @ClientNumber AS VARCHAR(50)) 
    RETURNS INT
    AS 
    BEGIN

    RETURN ISNULL((SELECT 0 FROM dbo.Client WHERE dbo.Client.ClientTypeID = @ClientTypeID AND dbo.Client.ClientNumber = @ClientNumber), 1)

    END

     


Log in to reply