Is my SProc written correctly?

I am very new to SQL server and I'm using stored procedures for my program. So far I wrote one tonight, that works just fine.

I haven't really written one before, but its kind of similar syntax since I know C++/C# and VB.

My question is, even though this works for what I need it do, is it written correctly? Can you see any problems with it, or would you have done it differently?

I want to make sure its done correctly, and it runs as fast as possible.

Thanks!

[pre]

CREATE PROCEDURE CreateNewUser

@.UserID int out,
@.LoginID nvarchar(30),
@.Password nvarchar(30),
@.RegisterDate smalldatetime,
@.LoginIDExists nvarchar(30)
AS
/* Check to see if the loginID is already in use before attempting to save it.
We MUST have a unique loginID for each user */
SELECT @.LoginIDExists = loginID FROM users WHERE loginID = @.LoginID

/* If we pulled a value from the database, then the loginID already exists, return with error code 1 */
IF (@.LoginIDExists = @.LoginID)

BEGIN
SELECT 1
RETURN
END

ELSE BEGIN
/* The loginID does not already exist, attemp to add the new user to the database. */
INSERT INTO users (
loginID,
loginpassword,
registerDate )
VALUES (
@.LoginID,
@.Password,
@.RegisterDate )

/* Grab the UserID for the new user. */
SELECT @.UserID = @.@.identity

/* return with error code 0 */
SELECT 0

RETURN

END
GO

[/pre]

I think it's ok, only some suggestions:

Replace the @.@.IDENTITY toSCOPE_IDENTITY()

Instead of using the SELECT statement to return the error code, you can use the RETURN statement. If you use the RETURN statement, you need to add a parameter and set the ParameterDirection to ReturnValue to the ADO.Net Command object.

|||Personally, I'd simplify it a bit more by removing the @.LoginExistsparameter. I'd take Fredrik's suggestions, plus I'd do the RETURNvalues slightly differently, and I'd do an @.@.ERROR check:

CREATE PROCEDURE CreateNewUser
@.UserID int OUTPUT,
@.LoginID nvarchar(30),
@.Password nvarchar(30),
@.RegisterDate smalldatetime
AS
DECLARE @.Result INT

/* Check to see if the loginID is already in use before attempting to save it.
We MUST have a unique loginID for each user */
IF EXISTS(SELECT loginID FROM users WHERE loginID = @.LoginID)

/* If we pulled a value from the database, then the loginID already exists, return with error code -1 */
BEGIN
SELECT @.RESULT = -1
END

ELSE
BEGIN
/* The loginID does not already exist, attempt to add the new user to the database. */
INSERT INTO users (
loginID,
loginpassword,
registerDate )
VALUES (
@.LoginID,
@.Password,
@.RegisterDate )

/* Grab the @.@.Error and the UserID for the new user. */

SELECT @.Result = @.@.Error, @.UserID = Scope_Identity()

END

/* @.Result = -1 if the user was already on file, 0 if the user was added, and something else if the
user was attempted to be inserted and the INSERT failed */

RETURN @.Result

|||What I just posted allows a gap between the database check and thedatabase insert where someone else could insert a record with thatloginID (your original code also allowed this). Below I've addeda TRASNACTION and an UPDLOCK which will prevent that from happening:

CREATE PROCEDURE CreateNewUser
@.UserID int OUTPUT,
@.LoginID nvarchar(30),
@.Password nvarchar(30),
@.RegisterDate smalldatetime
AS
DECLARE @.Result INT

/* Check to see if the loginID is already in use before attempting to save it.
We MUST have a unique loginID for each user */

BEGIN TRANSACTION
IF EXISTS(SELECT loginID FROM usersWITH (UPDLOCK) WHERE loginID = @.LoginID)

/* If we pulled a value from the database, then the loginID already exists, return with error code -1 */
BEGIN
SELECT @.RESULT = -1
END

ELSE
BEGIN
/* The loginID does not already exist, attempt to add the new user to the database. */
INSERT INTO users (
loginID,
loginpassword,
registerDate )
VALUES (
@.LoginID,
@.Password,
@.RegisterDate )

/* Grab the @.@.Error and the UserID for the new user. */

SELECT @.Result = @.@.Error, @.UserID = Scope_Identity()

END

/* @.Result = -1 if the user was already on file, 0 if the user was added, and something else if the
user was attempted to be inserted and the INSERT failed . COMMIT the transaction if there
were no errors, otherwise, ROLLBACK */

IF @.Result = 0
BEGIN
COMMIT
END
ELSE
BEGIN
ROLLBACK
END
RETURN @.Result|||

Thank's that's pretty cool. I need to get a book on SQL Server so I can learn to do all this stuff better.

I copied the code so I can modify my procedure later. I was going to make one procedure for each function, like CreateUser, EditUser, DeleteUser, ChangePassword, etc...

Do you think this is a good idea or should I include them all in one procedure.

I wanted to keep them small and simple. I didn't want to end up with a 100+ line procedure. I figured it would be better to do everything seperatly, and there's less code to run on each procedure so maybe they will be a little faster.

Thanks.

|||

Bluebarry wrote:

I copied the code so I can modify my procedure later. I was going tomake one procedure for each function, like CreateUser, EditUser,DeleteUser, ChangePassword, etc...

Personally, I like separate procedures. I think each storedprocedure should have one function. But if you have a lotof tables, that could mean a lot of lot of stored procedures.|||

tmorton wrote:

Bluebarry wrote:

I copied the code so I can modify my procedure later. I was going to make one procedure for each function, like CreateUser, EditUser, DeleteUser, ChangePassword, etc...

Personally, I like separate procedures. I think each stored procedure should have one function. But if you have a lot of tables, that could mean a lot of lot of stored procedures.

I dont care too much how many procedures i have unless its going to effect performance. i think having one large procedure would hurt performance more then several smaller procedures. That single procedure has to run more lines of code and do more calculations, like (If we are inserting to this, otherwise do this)... I write my programs in the same way I create many small simple methods rather then a few large methods. It makes it easier to keep bugs out of the program too.