current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 3 down vote favorite

I'm looking for following kind of feedback:

  • Firstly, is the use of a cursor an overkill here?
  • Is there a simpler way to do what I'm doing in SQL?
  • Could the script go terribly wrong?
  • I have limited SQL experience, how does it look? Any standards that I'm not following?

Bear in mind that this is a one-time script to be run, so performance is not absolutely crucial. Nevertheless, I would like to have some feedback on whether it could still be done more efficiently.

A few words about what I'm trying to do: There are two tables, tblEmployment & tblUserCodeList. The first table (tblEmployment) has a column of type int named RepaymentType. Due to a requirements change, this has to be abandoned and converted to a specific GUID/uniqueidentifier to be stored in another column RepaymentTypeIdentifier in the same tblEmployment. That is the whole point of the SQL script. I'll leave out further business logic.

So for each row in tblEmployment I want to read its RepaymentType and depending on its value (can only be 1-12) I want to store a specific GUID from tblUserCodeList (column:UserCodelistIdentifier) in the column RepaymentTypeIdentifier.

The column RepaymentTypeIdentifier is thus a Foreign Key referencing UserCodelistIdentifier in tblUserCodelist.

(I have shortened the CASE statement a little bit.)

USE db_name_left_out...
GO

DECLARE @EmploymentId as NVARCHAR(50);
DECLARE @UserCodeList TABLE
(
    Title NVARCHAR(100),
    UserCodeListIdentifier NVARCHAR(50)
)

INSERT INTO @UserCodeList (Title, UserCodeListIdentifier)
    SELECT Title, UserCodeListIdentifier 
    FROM tblUserCodeList;

Declare @EmpCursor as CURSOR

SET @EmpCursor = CURSOR FOR
    SELECT EmploymentIdentifier FROM dbo.tblEmployment;

OPEN @EmpCursor;

FETCH NEXT FROM @EmpCursor INTO @EmploymentId;

WHILE @@FETCH_STATUS = 0
BEGIN   
    UPDATE dbo.tblEmployment SET RepaymentTypeIdentifier = CASE 
            when RepaymentType = 1 then (SELECT TOP 1 UserCodeListIdentifier FROM @UserCodeList WHERE title like '%some_text_1%') 
            when RepaymentType = 2 then (SELECT TOP 1 UserCodeListIdentifier FROM @UserCodeList WHERE title like '%some_text_2%')
            when RepaymentType = 3 then (SELECT TOP 1 UserCodeListIdentifier FROM @UserCodeList WHERE title like '%some_text_3%')
            .
            .
            .
            when RepaymentType = 12 then (SELECT TOP 1 UserCodeListIdentifier FROM @UserCodeList WHERE title like '%some_text_12%')
        END
    WHERE EmploymentIdentifier = @EmploymentId

    FETCH NEXT FROM @EmpCursor INTO @EmploymentId;
END

CLOSE @EmpCursor

DEALLOCATE @EmpCursor;
share|improve this question

1 Answer 1

up vote 4 down vote accepted

This code can be simplified a lot, and in to a single update without the cursor. This may be a problem, though if your transaction log is not large enough to accommodate a mass update to all your records.

Right, how to simplify the update?

First, build the case statement in to the @UserCodeList logical table... Consider this:

INSERT INTO @UserCodeList (Title, UserCodeListIdentifier, RepaymentTypeToUpdate)
SELECT Title, UserCodeListIdentifier,
       case
           when title like '%some_text_1%' then 1
           when title like '%some_text_2%' then 2
           .....
       end
FROM tblUserCodeList;

Now, in that table, you have the correct RepaymentType cross reference for the update.

Your update now simply becomes:

UPDATE dbo.tblEmployment
SET RepaymentTypeIdentifier = (
    select UserCodeListIdentifier
    from @UserCodeList
    where RepaymentType = RepaymentTypeToUpdate
)

The above update can be done in a few different ways.... a CTE would be my preference:

with UpdateMap as (
    SELECT Title,
           UserCodeListIdentifier,
           case
               when title like '%some_text_1%' then 1
               when title like '%some_text_2%' then 2
               .....
           end as MatchRepayment
    FROM tblUserCodeList;
)
UPDATE dbo.tblEmployment
SET RepaymentTypeIdentifier = UserCodeListIdentifier
FROM UpdateMap
where MatchRepayment = RepaymentType

Having said all that, let's revisit your questions:

  1. Firstly, is the use of a cursor an overkill here?

    Probably. If your database transaction log is small, though, it may be better to break your updates in to batches that fit. A single large update may fail, and roll back, if there's not enough space to log it as a single operation. Your cursor breaks it doen to a sinlge record each time, and there's plenty of space for that, though. Ask your DBA, or just try it... if you are uncertain.

  2. Is there a simpler way to do what I'm doing in SQL?

    SQL is a set-based language. If you have a set-based way of thinking about it, then my suggestions are 'simpler'. Certainly, it is more concise.

  3. Could the script go terribly wrong?

    Yes.... it could. Take a backup first, and verify everything!

  4. I have limited SQL experience, how does it look? Any standards that I'm not following?

    Your conventions, for what you are doing, are reasonably good. There's very little in the way of 'standards' for SQL style... consistency is the key factor to look for.

share|improve this answer
    
Wow. That's one heck of an improvement. I never liked the use of a cursor in the first place. Talking about cursors, would you advice against using them unless absolutely necessary? –  D.Singh Dec 29 '14 at 1:17
    
I am a regular user of cursors, but only when necessary. Cursors are slow. Databases are designed to do things as sets, so using cursors go against the natural semantics of the database. Sometimes it's unavoidable. Not this time though. Note, I edited my answer, and, take a backup before you mess something up.... I have not tested my code... –  rolfl Dec 29 '14 at 1:21
    
Thank you for answering my questions specifically as well. Very helpful! I will be taking a back up and be very careful when running this script together with the (self- appointed) dba. Your point about databases being designed for set operations is something I need to keep in mind when writing SQL scripts in the future. I found a cursor much easier to understand and more similar to what I'm used to. But I'm aware of its slowness which led me to ask this question in the first place. One last thing: I haven't explicitly declared the foreign key relationship. Do I have to ? –  D.Singh Dec 29 '14 at 1:32
1  
THe FK relationship can be useful. It requires discipline and strict practice to maintain, and it can have some performance and logistical consequences. There are some significant advatages too. There are multiple schools-of-though about them. I'm not going to recommend one way vs. another. That's something you need local insight on whether you have a situation where the pro's and con's work out in one way or the other for you. It's seldom a clear-cut decision. –  rolfl Dec 29 '14 at 1:35
    
Thanks for your opinion on FK. FYI your suggested code works like a charm! Every row was updated fast and correctly. My original solution took ~ 14-15 seconds. Yours, only a few seconds. –  D.Singh Dec 29 '14 at 9:04

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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