Please start any new threads on our new site at https://forums.sqlteam.com. We've got lots of great SQL Server experts to answer whatever question you can come up with.

 All Forums
 SQL Server 2008 Forums
 Transact-SQL (2008)
 Is it the best way???

Author  Topic 

Cre8tivedaze
Starting Member

8 Posts

Posted - 2013-08-21 : 11:17:59
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) [url]http://technet.microsoft.com/en-us/library/aa175920%28v=sql.80%29.aspx[/url] 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
Master Smack Fu Yak Hacker

3873 Posts

Posted - 2013-08-21 : 11:29:58
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

8 Posts

Posted - 2013-08-21 : 11:37:03
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
Master Smack Fu Yak Hacker

4614 Posts

Posted - 2013-08-21 : 12:16:12
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
Go to Top of Page

Cre8tivedaze
Starting Member

8 Posts

Posted - 2013-08-21 : 12:28:12
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.
Go to Top of Page

Lamprey
Master Smack Fu Yak Hacker

4614 Posts

Posted - 2013-08-21 : 12:44:32
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
Master Smack Fu Yak Hacker

4614 Posts

Posted - 2013-08-21 : 12:57:53
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
Master Smack Fu Yak Hacker

3873 Posts

Posted - 2013-08-21 : 13:19:37
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

19 Posts

Posted - 2013-08-28 : 07:45:50
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
Master Smack Fu Yak Hacker

3873 Posts

Posted - 2013-08-28 : 08:41:18
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
   

- Advertisement -