Would someone be willing to critique a working Stored Procedure for me

[WantsToLearn]

Hello All,

I have a small client that has an Access front end to his SQL Server. I recently wrote a new Receiving form for him and rather than write SQL in the Access form code to update the linked tables, I created a Stored Procedure and called it using ADO from the Access code.

It is working very well, but I don't have any local technical resourses other than SQL Server books help. Would someone be kind enough to critique it for me from a style and eror checking perspective? If you would be kind enough to post an email address, I would be happy to send it to you. I would post it here but it is quite lengthy since it updates multiple tables. Thanks in advance for any help!

 

[donutman]

I'd prefer a post.
-Karl

 

[WantsToLearn]

OK, below is the code. Here are the impacted tables and what I am trying to do.

TblPO is the Purchase Order header file and has associated with it the corresponding line items in TblPoDetails. When an item is received a TblPOReceipts record is created and the QtyOnHand and QtyOnOrder fields in Products are increased and decreased by the QtyReceived.

Both TblPO and TblPoDetails have a ReceivedComplete and ReceivedPartial flag defined. If a received qty does not match the detail qty ordered, the ReceivedPartial flags are set for both tables.

A detail record is flagged complete when the accumulated received quantities meet or exceed the ordered quantity. A header record is flagged complete when all corresponding detail records are flagged complete.

I am trying to put it in a transaction so that all updates would have to be successful. I read somewhere that SQL Server checks the master table for sp_ so I used spc to save the lookup. Also, it sticks in my mind that you should always use dbo for things of this nature. Thanks for your willingness to help by looking at this.

************************* Code Begin *********************
/* Ensure we are using correct database */
USE Database1
GO
IF DB_NAME() <> 'Database1' BEGIN
RAISERROR('Wrong database',16,10)
RETURN
END

/* Delete the Stored Procedure if it exists */
IF OBJECT_ID('dbo.spcProcessTblPOReceipt') IS NOT NULL
DROP PROCEDURE dbo.spcProcessTblPOReceipt
GO

/* Stored Procedure for POReceipts */
CREATE PROCEDURE dbo.spcProcessTblPOReceipt
-- Input parms
@PO_ID int,
@PODet_ID int,
@ProductID int,
@NewQty float, -- Quantity received via Receiving form user entry
@DetailsQty float -- Quantity ordered from TblPoDetails record
AS
/* All changes to TblPOReceipts, TblPoDetails and TblPO must be valid */
BEGIN TRANSACTION

DECLARE @Err int,
@DetailsCount int,
@DetailsComplete int,
@TotalReceiptsQty float

/* Create a new record in TblPOReceipts */
INSERT INTO dbo.TblPOReceipts (PO_ID, PODet_ID, Quantity)
VALUES (@PO_ID, @PODet_ID, @NewQty)
SET @Err = @@ERROR
IF @Err <> 0 BEGIN
ROLLBACK TRANSACTION
RETURN(@Err)
END

/* Update quantity fields in Products table */
UPDATE dbo.Products
-- SET DateUpdated = getdate(), -- Per owner, system changes should NOT update
SET Qty_On_Hand = Qty_On_Hand + @NewQty,
Qty_On_Order = Qty_On_Order - @NewQty
WHERE Products.ProductID = @ProductID
SET @Err = @@ERROR
IF @Err <> 0 BEGIN
ROLLBACK TRANSACTION
RETURN(@Err)
END

/* Could be 1 record if received complete or multiple partial records */
SET @TotalReceiptsQty = (SELECT SUM(Quantity) FROM dbo.TblPOReceipts
WHERE dbo.TblPOReceipts.PODet_ID = @PODet_ID)
SET @Err = @@ERROR
IF @Err <> 0 BEGIN
ROLLBACK TRANSACTION
RETURN(@Err)
END

/* Flag complete if details order qty has been met or exceeded */
IF @TotalReceiptsQty >= @DetailsQty BEGIN

/* Flag details record as complete */
UPDATE dbo.TblPoDetails
SET ReceivedComplete = 1
WHERE dbo.TblPoDetails.PODet_ID = @PODet_ID
SET @Err = @@ERROR
IF @Err <> 0 BEGIN
ROLLBACK TRANSACTION
RETURN(@Err)
END

/* Count number of detail records for this PO */
SET @DetailsCount = (SELECT Count(*) FROM dbo.TblPoDetails
WHERE dbo.TblPoDetails.PO_ID = @PO_ID)
SET @Err = @@ERROR
IF @Err <> 0 BEGIN
ROLLBACK TRANSACTION
RETURN(@Err)
END

/* Count number of completed detail records for this PO */
SET @DetailsComplete = (SELECT Count(*) FROM dbo.TblPoDetails PD
WHERE PD.PO_ID = @PO_ID AND PD.ReceivedComplete = 1)
SET @Err = @@ERROR
IF @Err <> 0 BEGIN
ROLLBACK TRANSACTION
RETURN(@Err)
END

/* If all details records are complete then this PO is complete */
IF @DetailsCount = @DetailsComplete BEGIN
UPDATE dbo.TblPO
SET ReceivedComplete = 1
WHERE dbo.TblPO.PO_ID = @PO_ID
END
SET @Err = @@ERROR
IF @Err <> 0 BEGIN
ROLLBACK TRANSACTION
RETURN(@Err)
END
END

ELSE BEGIN -- New receipts qty is less than details ordered qty

/* Update TblPoDetails ReceivedPartial flag */
UPDATE dbo.TblPoDetails
SET ReceivedPartial = 1
WHERE dbo.TblPoDetails.PODet_ID = @PODet_ID
SET @Err = @@ERROR
IF @Err <> 0 BEGIN
ROLLBACK TRANSACTION
RETURN(@Err)
END

/* Update TblPO ReceivedPartial flag */
UPDATE dbo.TblPO
SET ReceivedPartial = 1
WHERE dbo.TblPO.PO_ID = @PO_ID
SET @Err = @@ERROR
IF @Err <> 0 BEGIN
ROLLBACK TRANSACTION
RETURN(@Err)
END
END

/* Everything worked OK */
COMMIT TRANSACTION
GO
************************ Code End ************************

 

[donutman]

I don't know about the business logic (cuz I don't want to spend the time on it), but the SQL looks great to me. I'd be curious to hear from others about what the Begin/Commit/Rollback brings to the table within a SP. None of the error checking goes beyond the success or failure of the SP as far as I can tell. If a SP fails, then isn't everything left untouched (at least between GO statements)?
-Karl

 

[WantsToLearn]

So you're saying that a stored procedure is implicitly a transaction in itself and that the rollback will be automatic? Thanks for taking the time to respond!

 

[Siggy19]

It depends upon the setup of the database IIRC, but in general Stored Procs act immediately. Therefore on the following code;

UPDATE Table1
SET Field1a = 'A'
WHERE Field1b / Field1c > 1

UPDATE Table2
SET Field2a = 'A'
WHERE Field2b / Field2c > 1

If the first update fails (because a Field1c is zero), it will all be rolled back and the second update will not happen at all.

However, if the second update is the one that fails, the roll back will NOT normally rollback the first update's changes.

That is the advantage of putting the two updates in a single transaction.

 

[donutman]

Siggy, I need to check into that. What is IIRC? I've done a quick search on the web and BOL and find nothing relevant. When I'm back in the office I'll have to check how my system works, but I thought for sure all updates fail if 1 fails. Are you saying that this behavior can be set during installation?
-Karl

 

[WantsToLearn]

I think IIRC is If I Remember Correctly ;-)

 

[donutman]

Oooh!

 

[NathanGriffiths]

You could enhance the error checking a bit by using GOTO:
e.g.

DECLARE @process VARCHAR(100)

SET @process = ''
SET @Err = 0

.
.
.

 /* Create a new record in TblPOReceipts */
   INSERT INTO dbo.TblPOReceipts (PO_ID, PODet_ID, Quantity) 
                          VALUES (@PO_ID, @PODet_ID, @NewQty)
   SET @Err = @@ERROR
   IF @Err <> 0 BEGIN
      SET @process = 'Create a new record in TblPOReceipts'
      GOTO Commit_or_Rollback
   END

and then replace the COMMIT TRANSACTION with

Commit_or_Rollback:
IF @Err = 0
   COMMIT TRANSACTION
ELSE
  BEGIN
    ROLLBACK TRANSACTION
    RAISERROR('Stored procedure dbo.spcProcessTblPOReceipt caused an error, Error code: %d, in process %d', @Err, @process)
    RETURN
  END

Its a good idea to initialise variables e.g. SET @Err = 0 because if they have a value of NULL, comparisons using <> can produce unexpected results (e.g. the comparison NULL <> 0 can return FALSE depending on the setting of SET ANSI_NULLS).

Nathan


Want to get a good response to your question? Read this FAQ! -> faq183-874

 

[NathanGriffiths]

Sorry, that RAISERROR line should read;

RAISERROR('Stored procedure dbo.spcProcessTblPOReceipt caused an error, Error code: %d, in process %d', 10, 1, @Err, @process)

cheers

Nathan


Want to get a good response to your question? Read this FAQ! -> faq183-874

 

[WantsToLearn]

Thanks all for the feedback, I really appreciate it!