2
\$\begingroup\$

I'm doing an exercise, and would like to know if what I'm doing is correct and follows the correct structure for rollback in case a transaction fails. I would like to know as well if my try...catch statements are well implemented.

create procedure spTeste2(
    @tit1 bigint = 111,
    @tit2 bigint = 222
) as
begin try
if exists(select primeiroTitular, segundoTitular from ContasBancarias where (primeiroTitular = @tit1 and primeiroTitular is not null) and (segundoTitular = @tit2 and segundoTitular is not null))
    begin
        declare @nib varchar(50) = (select nib from ContasBancarias where primeiroTitular = @tit1 and segundoTitular = @tit2)

        begin transaction
            update ContasBancarias set primeiroTitular = @tit2 where nib = @nib
            update ContasBancarias set segundoTitular = @tit1 where nib = @nib
        commit transaction
    end
else
    begin
        raiserror('Pelo menos um destes titulares não existe, ou não estão nesta ordem', 16, 1)
    end
end try
begin catch
    print ERROR_MESSAGE()

    IF @@TRANCOUNT > 0
        ROLLBACK transaction
end catch

The table ContasBancarias has the following structure:

nib
primeiroTitular
segundoTitular

This code aims to switch the value from primeiroTitular to segundoTitular, and vice versa.

For example, if on a particular row primeiroTitular contains the value 5678 and segundoTitular contains the value 9635, after this stored procedure is executed, primeiroTitular should have 9635 and segundoTitular should have 5678.

\$\endgroup\$
1
\$\begingroup\$

Overall, I think your handling of transaction commit/rollback in the try/catch block is reasonable.

Error handling

You may consider:

  1. adding a bit more information to your returning error in the catch block; for instance, you can use RAISERROR to customize your error message with a lot more information. I strongly recommend to read that page on MSDN.

  2. finding a better method than just printing the output to the console if an error occurs; after all, console output is very ephemeral and can get lost easily. Instead, you could log errors to a table in the database, along with some information about how it was called and with what parameters.


Premise

The premise behind the way you wrote this stored procedure is a bit strange. Your goal is to swap the account holders on one account. Your are accomplishing this by requiring both of the account holders' ID and then if an account exists where X is primary and Y is secondary, you swap them.

I think it would make more sense to additionally take in the account number nib as a parameter, and verify that the primary account holder on the account matches what is input, and likewise the secondary also matches, and if they don't, raise a different kind of error stating that the account holders input do not match the account number provided.


  • You can combine your 2 UPDATE statements into one, since they are both updating the same row in the table.

  • You also don't need to use your input parameters to make the update, but you can keep it that way if you feel it makes the intention more clear. In my opinion it would be more clear (and possibly less error-prone) to just directly swap the values into one statement. Demo:

declare @Test table (rownum int, ID1 int, ID2 int);
insert into @Test (rownum, ID1, ID2) 
  values (1, 42, 99);
update @Test 
  set ID1 = ID2, 
      ID2 = ID1 
  where rownum = 1;
select * from @Test;
/* Result: ***
rownum ID1 ID2 
------ --- --- 
1      99  42  
**************/
  • Your naming of parameters should reflect what values you are expecting. @tit1 and @tit2 are always used to refer to primeiroTitular and segundoTitular, respectively. Why not just name them that?

  • The null-checks in your IF EXISTS clause are unnecessary, since your procedure parameters are not nullable, and as such the equals operator necessarily implies that the fields will not be null.

  • Some line breaks to shorten your long lines helps readability.

  • Some will disagree, but it is of my personal opinion that using semicolons ; to end statements makes things clearer. This is not required in SQL Server/Transact-SQL, but it is in almost every other database system.

  • Make sure to pick a name more descriptive than spTeste2 before you commit this into a production database :-)


Reformatted code:

create procedure spTeste2(
    @primeiroTitular bigint = 111,
    @segundoTitular bigint = 222
) as
begin try;
if exists (
        select primeiroTitular, segundoTitular 
        from ContasBancarias 
        where primeiroTitular = @primeiroTitular
          and segundoTitular = @segundoTitular
    )
    begin
        declare @nib varchar(50) = (
            select nib 
            from ContasBancarias 
            where primeiroTitular = @primeiroTitular 
            and segundoTitular = @segundoTitular
        );

        begin transaction;
            update ContasBancarias 
                set primeiroTitular = @segundoTitular,
                    segundoTitular = @primeiroTitular 
                where nib = @nib;
        commit transaction;
    end;
else
    begin
        raiserror('Pelo menos um destes titulares não existe, ou não estão nesta ordem', 16, 1);
    end
end try;
begin catch;
    print ERROR_MESSAGE();

    if @@TRANCOUNT > 0
        rollback transaction;
end catch;
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.