Criticize me, Please...

17y3.2k

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
==========

Thanks,
Ed.

4 Replies

Know the answer? Post it — somebody with the same question will find it here.

Vijaya Kadiyala17y ago
Hi

It is well documented...

Thanks

Vijaya Kadiyala

Http://dotnetvj.blogspot.com
0
Lisa17y ago
the procedure definetely looks good and needs no criticism. it does has a small error pointed out by Jeff regarding the copy no at the top.

it i should say is a well formed procedure handling all the exceptions and results

but i am not sure about the statement at top deleting the procedure if exits , i know its a best method, but i seldom use it . i feel its not quite a safe way to do.
overall , it looks a pretty good stored procedure

Thanks,
Lisa
0
Ed17y ago
It helps, thank you Jeff.

There won't be anyone else maintaining this app, as it is one that has been written solely for my portfolio, but your point is well taken, it should include some general comments for anyone reading it.

Thanks again for the critique.

Ed.
0
Jeff Bolton17y ago
Ed,

Overall the stored procedure looks good. You've taken the time to add comments, trap for errors, and add transaction support. It's difficult, however, to critique the logic behind the stored procedure without knowing the underlying database structure and the requirements for the code. I did see one apparent mistake:

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

Should read:

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

The only other suggestion I have would be to add a general comment to the top of the stored procedure that describes what it is trying to accomplish. That will go a long way toward helping the next developer who has to modify your code.

Hope that helps!

Jeff
0