I have created the following stored procedure which duplicates a record in a table and also all its related records in other tables however since I am a newbie to SQL I would appreciate it if someone could review my code and show me how it could be improved and whether there are any hazards.

   USE [Neptune2Dev]
    /****** Object:  StoredProcedure [dbo].[DuplicateItemInBatch]    Script Date: 03/04/2013 17:55:47 ******/
    ALTER PROCEDURE [dbo].[DuplicateItemInBatch] @batchID AS INT , @numInBatch AS INT


    declare @itemIdToBeDuplicated as int
    declare @duplicatedItemID as int
    declare @URI as varchar(max)
    declare @itemOrganisationID as int
    declare @duplicateItemOrganisationID as int


           --increment all proceeding numbInbatch fileds to accomodiate the new item being duplicated
           UPDATE items  SET numinbatch = numinbatch + 1
        WHERE numinbatch > @numInBatch AND BatchID = @batchID

           select @itemIdToBeDuplicated = ID from items i where i.BatchID = @batchID and i.NumInBatch = @numInBatch
           select @URI = URI from ItemMedia where ID = @itemIdToBeDuplicated

           --duplicate the item and insert it into the table
           insert into items
           i.SupplierID, i.Title, i.PubDate, i.Body, i.StatusID,
           i.IsRelevant, i.ClipDuration, i.ResearchDuration,
           i.Comments, i.ItemTypeID, i.PageNumber, i.BatchID,
           i.NumInBatch + 1, i.OverrideDate, i.MediaChannelID,
           i.Size, i.PreviouslyCompleted
           from items i where i.ID = @itemIdToBeDuplicated
           select @duplicatedItemID =  SCOPE_IDENTITY();

           select @itemOrganisationID = ID from ItemOrganisations where ItemID = @itemIdToBeDuplicated
           insert into ItemOrganisations
           select @duplicatedItemID, OrganisationID, comment, rating from ItemOrganisations where ID = @itemOrganisationID
           select @duplicateItemOrganisationID = SCOPE_IDENTITY();

           insert into ItemOrganisationMessages
           select @duplicateItemOrganisationID, MessageID from ItemOrganisationMessages where ItemOrganisationID = @itemOrganisationID

           insert into ItemOrganisationIssues
           select @duplicateItemOrganisationID, IssueID from ItemOrganisationIssues where ItemOrganisationID = @itemOrganisationID

           insert into ItemByLines
           select @duplicatedItemID, ByLineID from ItemByLines where ItemID = @itemIdToBeDuplicated

           insert into ItemDocumentLink
           select ItemDocumentID, @duplicatedItemID from ItemDocumentLink where ItemID = @itemIdToBeDuplicated

           insert into ProfileResults
           select ProfileId, @duplicatedItemID from ProfileResults where ItemID = @itemIdToBeDuplicated

           insert into ItemNotes
           select  @duplicatedItemID,Quote,Theme,Rating,Comment,IsBad from ItemNotes where ItemID = @itemIdToBeDuplicated

           IF @URI <> null
                  insert into ItemMedia values (@duplicatedItemID, @URI)
        PRINT  N'No Article to be duplicated '

        END TRY
            ERROR_NUMBER() +' ' + ERROR_MESSAGE()
  • \$\begingroup\$ Your comment for updating the numInBatch doesn't match (probably most) people's expectation for 'proceeding' (that is, that lower numbers come 'first'). Aside from that, you may have better luck by using float-point entries for sorting, because then you (usually) don't have to update every row during re-orders - you just place it midway between the destination. Beyond that, always list the columns you're inserting into (re-creating the table may change column order). See if your version of SQL Server allows selections from inserts (data-change references) \$\endgroup\$ Apr 3 '13 at 20:58

Here are some comments in no particular order:

  • Your CATCH block - including comments - is a copy and paste from the documentation. While it's always good to read the documentation, please remember that the examples are there to illustrate specific features and you shouldn't assume that they're usable as-is in production code
  • If an error occurs in the TRY block, your CATCH block commits if possible anyway, is this what you intended? XACT_STATE() tells you that the transaction can be committed, not that it should be. See this SO question for more information.
  • Your CATCH block doesn't examine or log the error message (using ERROR_NUMBER() and similar functions), so if an error does occur you will have no idea what it was
  • Always list all column names in an INSERT; this gives you some security if the table structure changes
  • You cannot compare NULL values with the standard comparison operators, you need to write IF @URI IS NULL instead
  • Always use schema names to reference objects in code
  • Using BEGIN and END to wrap the entire stored procedure body is not required but usually good practice, otherwise it's easy to forget where the procedure ends and accidentally add some code that you didn't mean to
  • Most procedures should SET NOCOUNT ON at the beginning

And please always mention your SQL Server version (2005, 2008, 2008R2, 2012) and edition (Standard, Enterprise, Express) because that determines which language features are available.

  • \$\begingroup\$ thanks for the reply, Ive edited the catch block, will this cause the transaction to roll back if there is an error? \$\endgroup\$
    – xerxes
    Apr 4 '13 at 21:21
  • \$\begingroup\$ Yes, if the transaction can be rolled back (which was the point of using XACT_STATE(). But note that not all errors can be caught. \$\endgroup\$
    – Pondlife
    Apr 5 '13 at 13:44

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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