# How can I improve the following stored procedure?

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]
GO
/****** Object:  StoredProcedure [dbo].[DuplicateItemInBatch]    Script Date: 03/04/2013 17:55:47 ******/
SET ANSI_NULLS ON
GO
SET QUOTED_IDENTIFIER ON
GO
ALTER PROCEDURE [dbo].[DuplicateItemInBatch] @batchID AS INT , @numInBatch AS INT

AS

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

BEGIN TRY
BEGIN TRANSACTION;

--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
select
i.SupplierID, i.Title, i.PubDate, i.Body, i.StatusID,
i.IsRelevant, i.ClipDuration, i.ResearchDuration,
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

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
BEGIN
insert into ItemMedia values (@duplicatedItemID, @URI)
END
ELSE
PRINT  N'No Article to be duplicated '

COMMIT TRANSACTION;
END TRY
BEGIN CATCH
BEGIN
PRINT
ERROR_NUMBER() +' ' + ERROR_MESSAGE()
ROLLBACK TRANSACTION;
END;
END CATCH;

• 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) 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.

• thanks for the reply, Ive edited the catch block, will this cause the transaction to roll back if there is an error? Apr 4 '13 at 21:21
• 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. Apr 5 '13 at 13:44