Is it necessary to put transactions in this code?

Question

my supervisor asked me to not put transactions and commit etc in this code because he says that it's useless to put transactions in this procedure. He's well experienced and i can't directly argue with him so need your views on it ?

ALTER PROCEDURE [Employee].[usp_InsertEmployeeAdvances](
      @AdvanceID        BIGINT,
      @Employee_ID      INT,
      @AdvanceDate      DATETIME,
      @Amount           MONEY,
      @MonthlyDeduction MONEY,
      @Balance          MONEY,
      @SYSTEMUSER_ID    INT,
      @EntryDateTime    DATETIME = NULL,
      @ProcedureType    SMALLINT)
AS
BEGIN
    BEGIN TRY
        BEGIN TRANSACTION [Trans1]
        IF EXISTS
           (
              SELECT *
              FROM Employee.Advance
              WHERE AdvanceID = @AdvanceID
           )
        BEGIN
            --UPDATION OF THE RECORD
            IF @ProcedureType = 1
            BEGIN
                SET @Amount = @Amount * -1;
            END
            UPDATE Employee.Advance
            SET
                  Employee_ID = @Employee_ID,
                  AdvanceDate = @AdvanceDate,
                  Amount = @Amount,
                  MonthlyDeduction = @MonthlyDeduction,
                  Balance = @Balance,
                  EntryDateTime = GETDATE()
            WHERE AdvanceID = @AdvanceID
        END
        ELSE
        BEGIN
            DECLARE @LastRecordID INT
            DECLARE @LastBalance MONEY
            SET @LastRecordID =
               (
                  SELECT MAX(EA.AdvanceID)
                  FROM Employee.Advance EA
                  WHERE EA.Employee_ID = @Employee_ID
               )
            SET @LastBalance =
               (
                  SELECT EA.Balance
                  FROM Employee.Advance EA
                  WHERE EA.AdvanceID = ISNULL(@LastRecordID, 0)
               )
            IF(@ProcedureType = 0) --Advances
            BEGIN
                SET @Balance = ISNULL(@LastBalance, 0) + @Amount
                INSERT INTO Employee.Advance
                                            (Employee_ID,
                                             AdvanceDate,
                                             Amount,
                                             MonthlyDeduction,
                                             Balance,
                                             User_ID,
                                             EntryDateTime
                                            )
                VALUES
                       (@Employee_ID,
                        @AdvanceDate,
                        @Amount,
                        @MonthlyDeduction,
                        @Balance,
                        @SYSTEMUSER_ID,
                        GETDATE())
            END
            ELSE --Receivings
            BEGIN
                IF NOT EXISTS
                   (
                      SELECT *
                      FROM Employee.Advance EA
                      WHERE EA.Employee_ID = @Employee_ID
                            AND EA.Balance > 0
                            AND EA.AdvanceID =
                         (
                            SELECT MAX(AdvanceID)
                            FROM Advance
                            WHERE Employee_ID = @Employee_ID
                         )
                   )
                BEGIN
                    RAISERROR('This Employee has no advances history', 16, 1)
                    RETURN
                    --Select 0                  
                END
                ELSE
                BEGIN
                    SET @Balance = ISNULL(@LastBalance, 0) - @Amount
                    INSERT INTO Employee.Advance
                                                (Employee_ID,
                                                 AdvanceDate,
                                                 Amount,
                                                 MonthlyDeduction,
                                                 Balance,
                                                 User_ID,
                                                 EntryDateTime
                                                )
                    VALUES
                           (@Employee_ID,
                            @AdvanceDate,
                            -1 * @Amount,
                            @MonthlyDeduction,
                            @Balance,
                            @SYSTEMUSER_ID,
                            GETDATE())
                END
            END
        END
        COMMIT TRANSACTION [Trans1]
    END TRY
    BEGIN CATCH
        ROLLBACK TRANSACTION [Trans1]
    END CATCH
END

Answer 1

As the stored procedure is written right now a transaction isn't going to add anything. Only one of the modifications will ever run and if it fails then it will roll itself back anyway. Also of importance to note is that nothing happens after the modifications. If there were some additional code after one of the updates then that would likely necessitate a transaction.

However, what is the cost of having that transaction? Do you expect that this code will never change? A change in the future might make a transaction necessary without it being obvious. Given the nature of your stored procedure I'm finding it hard to believe that there's a downside to the transaction being there.

To handle RAISERROR inside of your TRY..CATCH block:

BEGIN TRANSACTION

BEGIN TRY


    COMMIT TRANSACTION
END TRY
BEGIN CATCH
    IF (@@TRANCOUNT > 0)
        ROLLBACK TRANSACTION

    EXEC dbo.LogErrorAndRethrow
END CATCH

The code for LogErrorAndRethrow then looks like:

CREATE PROCEDURE dbo.LogErrorAndRethrow
    @LogError BIT = 1
AS
BEGIN
    DECLARE
        @error_number   INT,
        @error_message  NVARCHAR(MAX),
        @error_severity INT,
        @error_state    INT,
        @error_procedure NVARCHAR(200),
        @error_line     INT

    SELECT
        @error_number = ERROR_NUMBER(),
        @error_message = ERROR_MESSAGE(),
        @error_severity = ERROR_SEVERITY(),
        @error_state = ERROR_STATE(),
        @error_procedure = COALESCE(ERROR_PROCEDURE(), '<Unknown>'),
        @error_line = ERROR_LINE()

    IF (@LogError = 1)
    BEGIN
        EXEC dbo.InsertAuditLog
            @UserID = 0,
            @AuditType = 'Error',
            @ModuleName = @error_procedure,
            @Message = @error_message
    END

    -- Rebuild the error message with parameters so that all information appears in the RAISERROR
    SELECT @error_message = N'Error %d, Level %d, State %d, Procedure %s, Line %d, ' + 'Message: ' + ERROR_MESSAGE()

    RAISERROR(@error_message, @error_severity, 1, @error_number, @error_severity, @error_state, @error_procedure, @error_line)
END

InsertAuditLog basically just inserts into a log table.

Answer 2

This is a changed answer as I did not read the whole question

Without a transaction if this was called at the same time and record did not exists then both could insert and one would likely get @Balance wrong

yes a transaction serves a purpose

Answer 3

I'd never put transactions in a particular bit of work simply because you never know when it is going to be incorporated into another. Only the caller can know that. To use the well worn example, you might think that creating an order and adding items should be a transaction which is fair enough. Down the track some calling function might want to include the results of a credit check or account creation as part of it. So basically at the low level you cannot know* if you're in the context of a transaction or not so there's not much point in being strict about committing. Similarly, rolling back is a bit wrong - you never know how fatal a specific error is in the context of the caller, so just throw the exception and let the client decide how to manage it.

*well you can, but it's often easier not to care.

Answer 4

ALTER PROCEDURE [Employee].[usp_InsertEmployeeAdvances]
(
      @AdvanceID        BIGINT,
      @Employee_ID      INT,
      @AdvanceDate      DATETIME,
      @Amount           MONEY,
      @MonthlyDeduction MONEY,
      @Balance          MONEY,
      @SYSTEMUSER_ID    INT,
      @EntryDateTime    DATETIME = NULL,
      @ProcedureType    SMALLINT
)
AS BEGIN

    SET NOCOUNT ON

    IF EXISTS (
        SELECT 1
        FROM Employee.Advance
        WHERE AdvanceID = @AdvanceID
    )
    BEGIN

        UPDATE Employee.Advance
        SET
                Employee_ID = @Employee_ID,
                AdvanceDate = @AdvanceDate,
                Amount = CASE WHEN @ProcedureType = 1 THEN -@Amount ELSE @Amount END,
                MonthlyDeduction = @MonthlyDeduction,
                Balance = @Balance,
                EntryDateTime = GETDATE()
        WHERE AdvanceID = @AdvanceID

    END
    ELSE BEGIN

        DECLARE
              @LastRecordID INT
            , @LastBalance MONEY
            , @IsBalance BIT

        SELECT @LastRecordID = MAX(AdvanceID)
        FROM Employee.Advance
        WHERE Employee_ID = @Employee_ID

        SELECT
            @LastBalance = Balance,
            @IsBalance = CASE WHEN Balance > 0 THEN 1 ELSE 0 END
        FROM Employee.Advance
        WHERE AdvanceID = ISNULL(@LastRecordID, 0)

        IF ISNULL(@IsBalance, 0) = 0 BEGIN

            RAISERROR('This Employee has no advances history', 16, 1)
            RETURN 

        END
        ELSE BEGIN

            INSERT INTO Employee.Advance(Employee_ID, AdvanceDate, Amount, MonthlyDeduction, Balance, [User_ID], EntryDateTime)
            SELECT
                @Employee_ID,
                @AdvanceDate,
                CASE WHEN @ProcedureType = 0 THEN @Amount ELSE -@Amount END,
                @MonthlyDeduction,
                ISNULL(@LastBalance, 0) + CASE WHEN @ProcedureType = 0 THEN @Amount ELSE -@Amount END,
                @SYSTEMUSER_ID,
                GETDATE()

        END

    END

END