SQL Server Forums
Profile | Register | Active Topics | Members | Search | Forum FAQ
 
Register Now and get your question answered!
Username:
Password:
Save Password
Forgot your Password?

 All Forums
 SQL Server 2008 Forums
 Transact-SQL (2008)
 Is it the best way???
 New Topic  Reply to Topic
 Printer Friendly
Author Previous Topic Topic Next Topic  

Cre8tivedaze
Starting Member

Australia
8 Posts

Posted - 08/21/2013 :  11:17:59  Show Profile  Reply with Quote
Hi all,

I've been using SQL on and off for probably about 10 years but nothing as advanced as the likes of some of you. I would probably rate myself somewhere between a beginner and Intermediate user but a recent project has seen me needing to really refine my sql skills. With that said, I've got this stored proc below that I put together using this tutorial (of sorts) http://technet.microsoft.com/en-us/library/aa175920%28v=sql.80%29.aspx mainly for it's error handling and proper code writing practices.

My question really is simple - it this statement massively over engineered or is pretty close to how someone who lives and breathes SQL everyday would consider a well structured SP. I'd appreciate any and all feedback.



CREATE PROCEDURE [dbo].[CreateJob]
	@TransactionCountOnEntry INT,
	@ErrorCode INT,
	@SPreturnVAL VARCHAR (20) OUTPUT,
	@userID UNIQUEIDENTIFIER,
	@creationDate DATETIME,
	@jobID INT,
	@TIfileName NVARCHAR(100),
	@jobNumber NVARCHAR(50),
	@authorName NVARCHAR(256),
	@authorID INT,
	@title NVARCHAR(256),
	@intervieweeID INT, 
	@intervieweeName NVARCHAR(256),
	@intervieweeCommonName NVARCHAR(256),
	@interviewerID INT, 
	@interviewerName NVARCHAR(256),
	@interviewerCommonName NVARCHAR(256),
	@statusID INT
AS
BEGIN
	SET NOCOUNT ON;
	
	SELECT @ErrorCode = @@ERROR
	
	IF @ErrorCode = 0
		BEGIN
			SELECT @TransactionCountOnEntry = @@TRANCOUNT
			BEGIN TRANSACTION
		END

		BEGIN
		
			SELECT * FROM NLA_JobRegister WHERE jobNumber = @jobNumber
	
			IF @@ROWCOUNT = 0

				BEGIN 
					SELECT @ErrorCode = @@ERROR
				
					IF @ErrorCode = 0 
					
						BEGIN
						
							BEGIN
								/* CHECK WHETHER OR NOT THE AUTHOR ALREADY EXISTS IN THE DB */
								SELECT * FROM Authors WHERE authorName = @authorName
								IF @@ROWCOUNT = 0
									SELECT @ErrorCode = @@ERROR
									IF @ErrorCode = 0
										BEGIN
											/* THE AUTHOR DOESN'T EXISTS SO INSERT IT AND RETURN ITS ID */
											INSERT INTO Authors VALUES (@authorName)
											SET		@authorID = SCOPE_IDENTITY()
											SELECT	@ErrorCode = @@ERROR
										END
								ELSE
									SELECT @ErrorCode = @@ERROR
									IF @ErrorCode = 0
										BEGIN
											/* THE AUTHOR ALREADY EXISTS SO GRAB ITS ID VALUE */ 
											SET		@authorID = (SELECT authorID FROM Authors WHERE authorName = @authorName)
											SELECT	@ErrorCode = @@ERROR
										END
							END
								
							BEGIN
								IF @ErrorCode = 0
									/* CHECK WHETHER OR NOT THE INTERVIEWEE ALREADY EXISTS IN THE DB*/
									SELECT * FROM Interviewee WHERE intervieweeName = @intervieweeName 
									
									IF @@ROWCOUNT = 0 
										/* INTERVIEWEE DOESN'T EXIST SO INSERT IT AND RETURN ITS ID */
										SELECT @ErrorCode = @@ERROR
										IF @ErrorCode = 0
											BEGIN
												INSERT INTO Interviewee VALUES (@intervieweeName, @intervieweeCommonName)
												SET		@intervieweeID = SCOPE_IDENTITY()
												SELECT	@ErrorCode = @@ERROR
											END
									ELSE
										SELECT @ErrorCode = @@ERROR
										IF @ErrorCode = 0
											BEGIN 
												/* THE INTERVIEWEE ALREADY EXISTS SO GRABS ITS ID VALUE */
												SET		@intervieweeID = (SELECT intervieweeID FROM Interviewee WHERE intervieweeName = @intervieweeName)
												SELECT	@ErrorCode = @@ERROR
											END
							END
								
							BEGIN
								IF @ErrorCode = 0
									/* CHECK WHETHER OR NOT THE INTERVIEWER ALREADY EXISTS IN THE DB */
									SELECT * FROM Interviewer WHERE interviewerName = @interviewerName
									
									IF @@ROWCOUNT = 0
										/* INTERVIEWER DOESN'T EXIST SO INSERT IT AND RETURN ITS ID */
										SELECT @ErrorCode = @@ERROR
										IF @ErrorCode = 0
											BEGIN
												INSERT INTOInterviewer VALUES (@interviewerName, @interviewerCommonName)
												SET		@interviewerID = SCOPE_IDENTITY()
												SELECT	@ErrorCode = @@ERROR
											END
									ELSE
										SELECT @ErrorCode = @@ERROR
										IF @ErrorCode = 0
											BEGIN
												/* THE INTERVIEWER ALREADY EXISTS SO GRABS ITS ID VALUE */
												SET		@interviewerID = (SELECT interviewerID FROM Interviewer WHERE interviewerName = @interviewerName)
												SELECT	@ErrorCode = @@ERROR
											END
							END
								
							BEGIN
								IF @ErrorCode = 0
									BEGIN
										/* CREATE A NEW RECORD IN THE JOB REGISTER */
										INSERT INTO	JobRegister
										VALUES		(@jobNumber, @TIfileName, @authorID, @title, @intervieweeID, @interviewerID, @creationDate, @userID, @statusID)
										SET			@jobID = SCOPE_IDENTITY()
										SELECT		@ErrorCode = @@ERROR
									END
							END
								
							BEGIN
								IF @ErrorCode = 0
									BEGIN
										/* CREATE A NEW LOG OF THE TRANSACTION */
										INSERT INTO Logging
										VALUES		(@jobID, @userID, 1, @creationDate)
										SELECT		@ErrorCode = @@ERROR
									END
							END
							
						END	
						
				END
				
			ELSE

				BEGIN
					SET @SPreturnVAL = 'Already Exists'
				END
		
		END
		
		BEGIN
		
			IF @@TRANCOUNT > @TransactionCountOnEntry
			
				BEGIN
					IF @ErrorCode = 0
						BEGIN
							SET @SPreturnVAL = 'Success'
							COMMIT TRANSACTION
						END
					ELSE
						BEGIN
							SET @SPreturnVAL = 'Something Died'
							ROLLBACK TRANSACTION
						END
				END
		
		END
		
END

James K
Flowing Fount of Yak Knowledge

3760 Posts

Posted - 08/21/2013 :  11:29:58  Show Profile  Reply with Quote
Two or three things from scanning through the code:

1. You could shorten the amount of code - for example, for the authors, something like this:
SET @authorId = NULL;
SELECT @authorId = authorId FROM Authors WHERE authorName = @authorName;

IF (@authorId IS NULL) 
BEGIN
	INSERT INTO Authors VALUES (@authorName);
	SET @authorId = SCOPE_IDENTITY();
END


2. Does this assume that authorName is always unique? Could there be two authors with the same name?

3. Since you are on SQL 2008, the preferred method for error handling is using TRY-CATCH blocks. There are examples here: http://msdn.microsoft.com/en-us/library/ms175976.aspx
Go to Top of Page

Cre8tivedaze
Starting Member

Australia
8 Posts

Posted - 08/21/2013 :  11:37:03  Show Profile  Reply with Quote
Hi James,

Good idea on the shortening - my head gets a bit lost in these somewhat nested stored procs.

As for the Author name always being unique, yes it will be.

Because I'm using SQL 2008 (it's actually 2008 R2) are you suggesting that maybe what I have put together isn't really optimal?


Cheers.
Go to Top of Page

Lamprey
Flowing Fount of Yak Knowledge

4614 Posts

Posted - 08/21/2013 :  12:16:12  Show Profile  Reply with Quote
Here are some thoughts; Instead of selecting the COUNT and then checking that value, it might be faster to do an IF NOT EXISTS. Since the EXISTS can and will short-circut (stop after finding a row) instead of having to count all the rows in the table. But, it looks like you expect the table to be empty anyway, so not a big deal.


AS James mentioned, TRY..CATCH

Here is another way to do your insert/populate AuthorID:
SET	@authorID = NULL;

----------

INSERT 
	Authors 
SELECT 
	@authorName
WHERE 
	NOT EXISTS
	(
		SELECT authorId 
		FROM Authors 
		WHERE authorName = @authorName;
	)

-- OR

MERGE 
	Authors AS Target
USING 
	(SELECT @authorName) AS Source (authorName)
	ON Target.authorName = Source.authorName
WHEN NOT MATCHED THEN
	INSERT (authorName)
	VALUES (Source.authorName);

---------------

SET	@authorID = SCOPE_IDENTITY()

IF @authorID IS NULL
BEGIN
	SET	@authorID = (SELECT authorID FROM Authors WHERE authorName = @authorName)
END

Edited by - Lamprey on 08/21/2013 12:17:02
Go to Top of Page

Cre8tivedaze
Starting Member

Australia
8 Posts

Posted - 08/21/2013 :  12:28:12  Show Profile  Reply with Quote
Thanks for the suggestions Lamprey.

I'm getting the feeling that TRY..CATCH blocks are getting the most support here.

So....Using a TRY..CATCH kind of operation, I could basically set a variable such as @DeadOrAlive in the CATCH block and set it to

SET @DeadOrAlive = 'Dead'

Then before executing the next block check that variable to see if it equals 'Alive', if it doesn't then pretty much continue to the end and rollback the transaction.

Edited by - Cre8tivedaze on 08/21/2013 12:29:57
Go to Top of Page

Lamprey
Flowing Fount of Yak Knowledge

4614 Posts

Posted - 08/21/2013 :  12:44:32  Show Profile  Reply with Quote
I'm not 100% sure what you need to handle, but I don't think you need to set/check anotehr variable. The basic form for using the TRY..CATCH is:
BEGIN TRAN

BEGIN TRY
	-- Do stuff
	COMMIT TRAN
END TRY

BEGIN CATCH
	ROLLBACK TRAN
END CATCH

You can get "more fancy" if you like and check the XACT_STATE:
IF (XACT_STATE()) = -1
BEGIN
    PRINT
        N'The transaction is in an uncommittable state.' +
        'Rolling back transaction.'
    ROLLBACK TRANSACTION;
END;

-- Test whether the transaction is committable.
IF (XACT_STATE()) = 1
BEGIN
    PRINT
        N'The transaction is committable.' +
        'Committing transaction.'
    COMMIT TRANSACTION;   
END;
Go to Top of Page

Lamprey
Flowing Fount of Yak Knowledge

4614 Posts

Posted - 08/21/2013 :  12:57:53  Show Profile  Reply with Quote
For Grins, here is a partial mock up of one way to handle things:
CREATE PROCEDURE [dbo].[CreateJob]
	@TransactionCountOnEntry INT,
	@ErrorCode INT,
	@SPreturnVAL VARCHAR (20) OUTPUT,
	@userID UNIQUEIDENTIFIER,
	@creationDate DATETIME,
	@jobID INT,
	@TIfileName NVARCHAR(100),
	@jobNumber NVARCHAR(50),
	@authorName NVARCHAR(256),
	@authorID INT,
	@title NVARCHAR(256),
	@intervieweeID INT, 
	@intervieweeName NVARCHAR(256),
	@intervieweeCommonName NVARCHAR(256),
	@interviewerID INT, 
	@interviewerName NVARCHAR(256),
	@interviewerCommonName NVARCHAR(256),
	@statusID INT
AS
BEGIN
	SET NOCOUNT ON;

	SET @TransactionCountOnEntry = @@TRANCOUNT

	BEGIN TRANSACTION

	BEGIN TRY
		IF NOT EXISTS (SELECT * FROM NLA_JobRegister WHERE jobNumber = @jobNumber)
		BEGIN
		
			SET	@authorID = NULL;

			MERGE 
				Authors AS Target
			USING 
				(SELECT @authorName) AS Source (authorName)
				ON Target.authorName = Source.authorName
			WHEN NOT MATCHED THEN
				INSERT (authorName)
				VALUES (Source.authorName);

			SET	@authorID = SCOPE_IDENTITY();


			IF @authorID IS NULL
			BEGIN
				SET	@authorID = (SELECT authorID FROM Authors WHERE authorName = @authorName);
			END
		END


		IF NOT EXISTS (SELECT * FROM Interviewee WHERE intervieweeName = @intervieweeName)
		BEGIN

			SET @intervieweeID = NULL;

			MERGE 
				Interviewee AS Target
			USING 
				(SELECT @intervieweeName, @intervieweeCommonName) AS Source (intervieweeName, intervieweeCommonName)
				ON Target.intervieweeName = Source.intervieweeName
			WHEN NOT MATCHED THEN
				INSERT (intervieweeName, intervieweeCommonName)
				VALUES (Source.intervieweeName, Source.intervieweeCommonName);
	
			SET	@intervieweeID = SCOPE_IDENTITY();

			IF @intervieweeID IS NULL
			BEGIN
				SET @intervieweeID = (SELECT intervieweeID FROM Interviewee WHERE intervieweeName = @intervieweeName);
			END
		END
		
		--- ETC..
		
		COMMIT TRANSACTION
	END TRY

	BEGIN CATCH
		ROLLBACK TRANSACTION
	END CATCH

END
Go to Top of Page

James K
Flowing Fount of Yak Knowledge

3760 Posts

Posted - 08/21/2013 :  13:19:37  Show Profile  Reply with Quote
More "for grins" type of stuff regarding MERGE, in particular, about a seemingly non-atomic behavior (which really is atomic for the default isolation level): http://sqlmag.com/sql-server/merge-statement-tips
Go to Top of Page

kameswararao polireddy
Starting Member

India
19 Posts

Posted - 08/28/2013 :  07:45:50  Show Profile  Reply with Quote
Using COUNT is better than using EXISTS or NOT EXISTS. Because count can be used in generating error messages at the end of the procedures.
Using MERGE is some what a risk some process because if you don’t get the indexing, joins and other considerations correctly, then it is possible to not only lose the performance benefits you hoped for, but to suffer from performance problems much much worse than if you had written the straight-forward INSERT, UPDATE and/or DELETE statements in the first place. Because of their complexity, I believe MERGE statements seem more vulnerable to bad query plans than other DML statements.
SO it is better to use INNER or LEFT JOINS instead of MERGE statements.

P.Kameswara rao
Go to Top of Page

James K
Flowing Fount of Yak Knowledge

3760 Posts

Posted - 08/28/2013 :  08:41:18  Show Profile  Reply with Quote
quote:
Originally posted by kameswararao polireddy

Using COUNT is better than using EXISTS or NOT EXISTS. Because count can be used in generating error messages at the end of the procedures.
Using MERGE is some what a risk some process because if you don’t get the indexing, joins and other considerations correctly, then it is possible to not only lose the performance benefits you hoped for, but to suffer from performance problems much much worse than if you had written the straight-forward INSERT, UPDATE and/or DELETE statements in the first place. Because of their complexity, I believe MERGE statements seem more vulnerable to bad query plans than other DML statements.
SO it is better to use INNER or LEFT JOINS instead of MERGE statements.

P.Kameswara rao

While there may be cases where one needs to use COUNT over EXISTS or NOT EXISTS, in many cases, it is beneficial to use EXISTS or NOT EXISTS. As always, it depends on the situation, but I lean in favor of exists/not exists over count in many cases. I can go over the reasons, but it has been well-documented and studied, for example: http://sqlblog.com/blogs/andrew_kelly/archive/2007/12/15/exists-vs-count-the-battle-never-ends.aspx

Regarding the statements you made about merge - can you point me to any documentation that backs up what you are saying?
Go to Top of Page
  Previous Topic Topic Next Topic  
 New Topic  Reply to Topic
 Printer Friendly
Jump To:
SQL Server Forums © 2000-2009 SQLTeam Publishing, LLC Go To Top Of Page
This page was generated in 0.11 seconds. Powered By: Snitz Forums 2000