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
 General SQL Server Forums
 New to SQL Server Programming
 My First Complex SP - Feedback Please

Author  Topic 

WeeBubba
Starting Member

18 Posts

Posted - 2006-12-08 : 19:55:19
hi. ive just written my first complex SP for SQLServer 2005. id be REALLY grateful for any feedback please. if there's anything i'm doing wrong then it's better that i know sooner rather than later! im wondering if its over complex or just plain wrong. please bear in mind that i have at least made an attempt at this, this is my first proper SP.

the SP that I have written is to insert an Account and a User. The SP should do the following.

1. Start a transaction.
2. Insert a user by calling a nested SP. if the return value is greater than zero then the user has been successfully inserted.
3. only attempt to insert an account afterwards if the user has been inserted successfully.
4. if the user or account insert is not successful then rollback. otherwise commit the transaction
5. return an error code from the SP as such: 0 = success. 1 = failure, a user already exists with the given username. 2 = an unknown error occurred whilst inserting the user. 3 = an unknown error occurred when inserting the Account.

here is my code:

CREATE PROCEDURE sp_Account_I
@Id_Package smallint,
@Amount money,
@Username varchar(16),
@Password varchar(88),
@FirstName varchar(32),
@Surname varchar(32),
@DateBirth datetime,
@Email varchar(64),
@Id_Country int,
@Id_Account int OUTPUT,
@Id_User int OUTPUT

AS

DECLARE @ErrorCode int;

BEGIN

SET NOCOUNT ON;

SELECT @Id_Account = -1;

BEGIN TRANSACTION

/* Insert the user. */
EXEC @ErrorCode = sp_User_I1
@Username,
@Password,
@FirstName,
@Surname,
@DateBirth,
@Email,
@Id_Country,
@Id_User OUTPUT

IF @ErrorCode > 0
BEGIN TRY
INSERT INTO Accounts(
Id_Package,
Id_UserMain,
Amount)
VALUES(
@Id_Package,
@Id_User,
@Amount)

SELECT @Id_Account = SCOPE_IDENTITY();

END TRY
BEGIN CATCH
SELECT @ErrorCode = 3; /* An unknown error occurred. */
END CATCH;

IF @ErrorCode = 0
COMMIT TRANSACTION
ELSE
BEGIN
ROLLBACK TRANSACTION
RETURN @ErrorCode;
END

END

Kristen
Test

22859 Posts

Posted - 2006-12-09 : 01:36:00
"if the return value is greater than zero then the user has been successfully inserted."

Don't like that. Return ZERO for success and anything else as an indicator of what went wrong - often @@ERROR

"return an error code from the SP as such: 0 = success. 1 = failure,"

is fine, but you've swapped policy on the AddUser SP!!

CREATE PROCEDURE dbo.sp_Account_I
-- Do NOT prefix your Stored Procedures by "SP_" as this [prefix alone] is very inefficient
@Id_Package smallint,
@Amount money,
@Username varchar(16),
@Password varchar(88), -- Why 88 characters? Its a monsterous password!
@FirstName varchar(32), -- Why multiples of 2? Users are more likely to expect round-number limits - like 30
@Surname varchar(32),
@DateBirth datetime,
@Email varchar(64), -- Too short for real world
@Id_Country int,
@Id_Account int OUTPUT,
@Id_User int OUTPUT
AS

DECLARE @ErrorCode int;
-- ";" not required, remove from all subsequent lines

BEGIN

SET NOCOUNT ON;
-- Move this to the top, immediately under "AS"

SELECT @Id_Account = -1;

BEGIN TRANSACTION

/* Insert the user. */
EXEC @ErrorCode = dbo.sp_User_I1 -- Same comment about the SP_ prefix
-- In order to avoid the parameters being position-dependant, particularly if sp_User_I1 changes, use:
@Username = @Username, etc.
@Password,
@FirstName,
@Surname,
@DateBirth,
@Email,
@Id_Country,
@Id_User = @Id_User OUTPUT -- I think you can only use an output for a named parameter

IF @ErrorCode > 0 AND @@ERROR = 0
BEGIN TRY
INSERT INTO Accounts(
Id_Package,
Id_UserMain,
Amount)
VALUES(
@Id_Package,
@Id_User, -- Inconsistent naming, use @Id_UserMain
@Amount)

SELECT @Id_Account = SCOPE_IDENTITY();

END TRY
BEGIN CATCH
SELECT @ErrorCode = 3; /* An unknown error occurred. */
END CATCH;

IF @ErrorCode = 0
COMMIT TRANSACTION
ELSE
BEGIN
-- Without nested named transactions this will most likely cause the application to fail
-- complaining that the transaction level is unexpected - force an error and see what happens!

ROLLBACK TRANSACTION
RETURN @ErrorCode;
END
-- No value is explicitly being returned for the "success" outcome
END

Kristen
Go to Top of Page

robvolk
Most Valuable Yak

15732 Posts

Posted - 2006-12-09 : 12:08:33
I agree with Kristen's points, and have two more suggestions:

1) Date of birth could be a smalldatetime, but there's no point changing the variable if you don't change the table. It's a minor issue, but the additional capacity and space used by datetime is wasted for such dates.

2) Instead of using status codes to return error conditions, better to use RAISERROR and return actual errors. ADO and ADO.Net have built-in mechanisms for retrieving these errors from the database, and you don't have to know what the numeric return codes mean. You can generate an error message with the exact text you want.
Go to Top of Page

jezemine
Master Smack Fu Yak Hacker

2886 Posts

Posted - 2006-12-09 : 16:58:45
quote:
Originally posted by robvolk
2) Instead of using status codes to return error conditions, better to use RAISERROR and return actual errors. ADO and ADO.Net have built-in mechanisms for retrieving these errors from the database, and you don't have to know what the numeric return codes mean. You can generate an error message with the exact text you want.



Another point in favor of RAISERROR: clients can't ignore you if you use RAISERROR, but if all you do is return an error code, they can ignore it and stumble forward into the darkness.


http://www.elsasoft.org
Go to Top of Page

spirit1
Cybernetic Yak Master

11752 Posts

Posted - 2006-12-09 : 18:37:46
also have a severity of 18 in raiserror if you want to cancel further execution of the batch.



Go with the flow & have fun! Else fight the flow
blog thingie: http://weblogs.sqlteam.com/mladenp
Go to Top of Page

WeeBubba
Starting Member

18 Posts

Posted - 2006-12-09 : 19:17:13
thanks for all your considerate replies. this is really helping me to learn and i can't thank you all enough. i am going to go off and write a second SP now. i will post it later to see if you think it is an improvement.

i have a couple of interim questions please.

1. why is a prefix of "sp_" inefficient?
2. when i use the RAISEERROR command, it is good that i can return an error string from the SP, such as "The username already exists." but i also need to return a code of some sort so that my calling C# code can determine what the next course of action should be. Is it possible for me to return a custom error code as well as a custom string with this command?

thanks
Go to Top of Page

robvolk
Most Valuable Yak

15732 Posts

Posted - 2006-12-09 : 23:09:11
1. sp_ is a special prefix in SQL Server, normally reserved for system procs. It assumes the procedure is stored in the master database, and will always look there first. It also makes it harder to determine which procedures are true system procs and which ones belong to your database and/or application. If you have to prefix your procs, use anything except "sp_".

2. Not sure why your C# code can't read a string error message instead of a (meaningless) number. However, if you have to have a code you can prefix the message with a numeric code that the app can parse:

RAISERROR('123 - The user account already exists.', 15,1)
Go to Top of Page

Kristen
Test

22859 Posts

Posted - 2006-12-10 : 01:00:39
We put

SET XACT_ABORT ON

at the top of all our Sprocs so that if we miss an error check the SProc will abort, rather than run further commands. You may want to consider that.

You may also want to consider

SET ARITHABORT ON

we've had so much trouble when we've introduced some types of views - views with INSTEAD OF triggers maybe? and some other things that I've now forgotten about - Computed columns maybe? - that we decided to put SET ARITHABORT ON in all our sprocs to lessen the collateral damage. Easier to do that from day one "just in case". But others may have different views.

Kristen
Go to Top of Page

WeeBubba
Starting Member

18 Posts

Posted - 2007-01-05 : 22:24:47
hello. you may remember me and this post! you may not! :)

i have worked hard and have produced 2 more SP's. if i show them once more i would be glad to hear your comments. it is better for me to know about any bad practices early on so that i can move forward with confidence.

the aim of these SP's is such. i have one SP to insert a user which must notify the client if a failure has occurred because the username already exists. i then have an 'insert account' SP. this inserts a user then inserts an account afterwards. it must be able to rollback if there is a problem. hopefully this is a big improvement on my last effort. im not sure if my error handling is overly complicated. many thanks for your time:

INSERT USER
------------------------------------------------------

...

AS

SET NOCOUNT ON

DECLARE @Error int

BEGIN

BEGIN TRY

INSERT INTO Users(
Username,
Password,
FirstName,
Surname,
DateBirth,
Email,
Id_Country)
VALUES(
@Username,
@Password,
@FirstName,
@Surname,
@DateBirth,
@Email,
@Id_Country)

END TRY
BEGIN CATCH

SELECT @Error = @@ERROR

IF @Error = 2601
RAISERROR('The username already exists.', 15, 1)
ELSE
RAISERROR('An unknown error occurred.', 15, 1)

RETURN @Error

END CATCH

SELECT @Id_User = SCOPE_IDENTITY()

RETURN

END



INSERT ACCOUNT
------------------------------------------------------

...

AS

SET NOCOUNT ON

DECLARE @Error int

BEGIN

BEGIN TRANSACTION

/* Insert the user. */
EXEC @Error = User_I1
@Username,
@Password,
@FirstName,
@Surname,
@DateBirth,
@Email,
@Id_Country,
@Id_User OUTPUT

IF @Error != 0
BEGIN

ROLLBACK TRANSACTION

RETURN @Error

END

IF @Error = 0
BEGIN TRY

SELECT @Error = 1/0

INSERT INTO Accounts(
Id_Package,
Id_UserMain,
Amount)
VALUES(
@Id_Package,
@Id_User,
@Amount)

SELECT @Id_Account = SCOPE_IDENTITY()

END TRY
BEGIN CATCH

SELECT @Error = @@ERROR

ROLLBACK TRANSACTION

RAISERROR('An unknown error occurred while inserting the account.', 15, 1)

RETURN @Error

END CATCH;

COMMIT TRANSACTION

RETURN

END
Go to Top of Page
   

- Advertisement -