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 2005 Forums
 Transact-SQL (2005)
 Criticize me, please...

Author  Topic 

EDanaII
Starting Member

5 Posts
Posted - 2009-02-13 : 11:37:15
I would appreciate a critique on the following stored procedure, if anyone is willing:

USE EDLibrary
GO
/****** Object:  StoredProcedure Items.CheckIn    Script Date: 01/30/2009 11:03:54 ******/
IF  EXISTS (SELECT * FROM sys.objects WHERE object_id = OBJECT_ID(N'Items.CheckIn') AND type in (N'P', N'PC'))
  DROP PROCEDURE Items.CheckIn
GO

/****** Object:  StoredProcedure Items.CheckIn    Script Date: 01/30/2009 11:04:10 ******/
SET ANSI_NULLS ON
GO
SET QUOTED_IDENTIFIER ON
GO

CREATE PROC Items.CheckIn (
  @ISBN     INT,
  @CopyNo   INT,
  @Result   INT OUTPUT
)
AS
SET NOCOUNT ON
--Do we have ISBN?
IF (@ISBN IS NULL)
  RAISERROR('An ISBN must be supplied.', 16, 1)

--Do we have Copy No?
IF (@ISBN IS NULL)
  RAISERROR('A Copy Number must be supplied.', 16, 2)

BEGIN TRY
  BEGIN TRANSACTION
--  Record this loan for posterity.
    INSERT INTO LoanHist
    SELECT ISBN, Copy_No, Out_Date, Title_No, Member_No, Due_Date, GETDATE(), NULL, NULL, NULL, NULL
    FROM Loan
    WHERE ISBN    = @ISBN
    AND   Copy_No = @CopyNo

--  Check to see if the update was successful.
    SET @Result = @@ERROR
    IF @Result <> 0
      RAISERROR('The LoanHist table was not updated.', 16, @Result)

--  Remove current loan from the record.
    DELETE FROM Loan
    WHERE ISBN    = @ISBN
    AND   Copy_No = @CopyNo

--  Check to see if the update was successful.
    SET @Result = @@ERROR
    IF @Result <> 0
      RAISERROR('The Loan table was not updated.', 16, @Result)

--  Indicate that the copy is no longer on loan.
    UPDATE Copy
    SET On_Loan = 'N'
    WHERE ISBN    = @ISBN
    AND   Copy_No = @CopyNo

--  Check to see if the update was successful.
    SET @Result = @@ERROR
    IF @Result <> 0
      RAISERROR('The Copy table was not updated.', 16, @Result)

    SET @Result = 0
  COMMIT TRANSACTION
END TRY

--Report on any errors should they occur.
BEGIN CATCH
  SET @Result = error_state()
  SELECT @Result 'Error State', error_message() 'Error Message'
  ROLLBACK TRANSACTION
END CATCH


Anything I'm doing wrong, anything I could do better? I'm just looking for anything that would help me improve my technique.

Thanks,
Ed.

yosiasz
Master Smack Fu Yak Hacker

1635 Posts
Posted - 2009-02-13 : 12:08:28
looks gorgeous to me

but you haev a problem in the second IF statement

--Do we have ISBN?
IF (@ISBN IS NULL)
RAISERROR('An ISBN must be supplied.', 16, 1)

--Do we have Copy No?
IF (@ISBN IS NULL)
RAISERROR('A Copy Number must be supplied.', 16, 2)
Go to Top of Page

Transact Charlie
Master Smack Fu Yak Hacker

3451 Posts
Posted - 2009-02-13 : 12:20:29
Might want to put some a RETURN statement after the messages as well so the you don't do the logic: That way you can also return an error value to a calling proc. (in this caase 500)


IF (@ISBN IS NULL)
  RAISERROR('An ISBN must be supplied.', 16, 1)


to 

IF (@ISBN IS NULL) BEGIN
  RAISERROR('An ISBN must be supplied.', 16, 1)
  RETURN 500
END



Charlie
===============================================================
Msg 3903, Level 16, State 1, Line 1736
The ROLLBACK TRANSACTION request has no corresponding BEGIN TRANSACTION
Go to Top of Page

Transact Charlie
Master Smack Fu Yak Hacker

3451 Posts
Posted - 2009-02-13 : 12:24:28
Also -- you make a blind reference to a table during an insert: I'd change that to include the columns inserted into -- it's easier to debug and it means the statement can survive a schema change that introduces optional columns. 

INSERT INTO LoanHist ( x, y, z, a, b, c, .....)
    SELECT ISBN, Copy_No, Out_Date, Title_No, Member_No, Due_Date, GETDATE(), NULL, NULL, NULL, NULL
    FROM Loan
    WHERE ISBN    = @ISBN
    AND   Copy_No = @CopyNo



Charlie
===============================================================
Msg 3903, Level 16, State 1, Line 1736
The ROLLBACK TRANSACTION request has no corresponding BEGIN TRANSACTION
Go to Top of Page

EDanaII
Starting Member

5 Posts
Posted - 2009-02-13 : 15:31:56
Thanks for the feedback, guys. It's appreciated.

@ Transact Charlie.

Wouldn't the RETURN be unreachable placed after the RAISERROR or am I misunderstanding how it works?

Also, wouldn't it be more efficient to place the RETURN at the end of the BEGIN CATCH block under those circumstances?

Just wondering.

Ed.
Go to Top of Page

Transact Charlie
Master Smack Fu Yak Hacker

3451 Posts
Posted - 2009-02-16 : 06:51:23
Hi EDanaII,

RAISERROR doesn't halt a batch (unless there's something I'm not aware of). However it does fire your TRY CATCH block. (I've just tested it on a 2005 server -- We've got a mixed 2000 / 2005 code base so we have to write to compant level 80 most of the time! Sucks because I can't use TRY / CATCH or my personal favourite ROW_NUMBER most of the time!)

Indded -- you don't need the RETURN value for each RAISERROR (you would without the TRY/CATCH and you could use it in your CATCH block after the ROLLBACK. Disregard my previous advice re RETURN -- it's not relevant!

all looks good -- I don't like unqualified references to tables for inserts / updates but that's probably a personal thing. It's only really bad if your schema is apt to change a lot.




Charlie
===============================================================
Msg 3903, Level 16, State 1, Line 1736
The ROLLBACK TRANSACTION request has no corresponding BEGIN TRANSACTION
Go to Top of Page

jbp_j
Starting Member

24 Posts
Posted - 2009-02-16 : 07:56:30
hi,


After any DML operation completed u r checking for error, if any error is occur the control automatically goes to catch block no need of Checking the error each time.

-- Check to see if the update was successful.
SET @Result = @@ERROR
IF @Result <> 0
RAISERROR('The LoanHist table was not updated.', 16, @Result)

if any errors are occurred u have to handle ERRORS in catch block only.

I THINK THIS CODE IS ENOUGH.

USE EDLibrary
GO
/****** Object: StoredProcedure Items.CheckIn Script Date: 01/30/2009 11:03:54 ******/
IF EXISTS (SELECT * FROM sys.objects WHERE object_id = OBJECT_ID(N'Items.CheckIn') AND type in (N'P', N'PC'))
DROP PROCEDURE Items.CheckIn
GO

/****** Object: StoredProcedure Items.CheckIn Script Date: 01/30/2009 11:04:10 ******/
SET ANSI_NULLS ON
GO
SET QUOTED_IDENTIFIER ON
GO

CREATE PROC Items.CheckIn (
@ISBN INT,
@CopyNo INT,
@Result INT OUTPUT
)
AS
SET NOCOUNT ON
--Do we have ISBN?
IF (@ISBN IS NULL) BEGIN
RAISERROR('An ISBN must be supplied.', 16, 1)
RETURN 500
END

--Do we have Copy No?
IF (@ISBN IS NULL) BEGIN
RAISERROR('A Copy Number must be supplied.', 16, 2)
RETURN 500
END


BEGIN TRY
BEGIN TRANSACTION
-- Record this loan for posterity.
INSERT INTO LoanHist
SELECT ISBN, Copy_No, Out_Date, Title_No, Member_No, Due_Date, GETDATE(), NULL, NULL, NULL, NULL
FROM Loan
WHERE ISBN = @ISBN
AND Copy_No = @CopyNo

-- Remove current loan from the record.
DELETE FROM Loan
WHERE ISBN = @ISBN
AND Copy_No = @CopyNo

-- Indicate that the copy is no longer on loan.
UPDATE Copy
SET On_Loan = 'N'
WHERE ISBN = @ISBN
AND Copy_No = @CopyNo

SET @Result = 0
COMMIT TRANSACTION
END TRY

--Report on any errors should they occur.
BEGIN CATCH
SET @Result = error_state()
SELECT @Result 'Error State', error_message() 'Error Message'
ROLLBACK TRANSACTION
END CATCH
Go to Top of Page
   

Subscribe to SQLTeam.com

SQLTeam.com Articles via RSS

SQLTeam.com Weblog via RSS

- Advertisement -

Resources

Articles

Forums

Blogs

Contact Us

About the Site