Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Using the following query I'm doing transactions with two tables

BEGIN TRANSACTION 
UPDATE SalaryTrans SET carried_forward_amount = @carriedForwardAmount, net_wage=@netWage, processed_date = @processedDate WHERE employee_id=@employeeID AND reference = @reference
DELETE FROM CarriedForward WHERE employee_id = @employeeID
INSERT INTO CarriedForward (employee_id, carried_forward_amount) VALUES(@employeeID, @carriedForwardAmount)
COMMIT;

First SalaryTrans table is updated. Then all matching records in CarriedForward table are deleted and new details are entered.

I just want to know whether this approach is acceptable or there are better ways to write this.

share|improve this question

2 Answers 2

up vote 5 down vote accepted

If you do it this way, how will you do an audit trail?

Of course I'm assuming you want that. It's just that accounting type problems tend to want that.

Have a think about a future statement from your system. Typically, you'd have a monthly statement with a "Carried Forward" at the bottom of each month. If you delete that, you won't be able to find it.

Suggestion: add a date column to the CarriedForward. Now you can also use that to make sure things are consistent between the salaries and the carries.

share|improve this answer
    
Actually I should have added that the CarriedForward table contains employees with their carried forward amounts. An employee only appears once in the table with his carried forward amount. Only thing happen is that value being changed monthly. –  Chathuranga Jul 28 at 16:35

In terms of the raw/basic functionality, what you have is fine. The transactional logic is good.

Readability is the only concern I have, and would rewrite your code as (note, there are some spaces I added around some = conditions):

BEGIN TRANSACTION 

    UPDATE SalaryTrans
    SET carried_forward_amount = @carriedForwardAmount,
        net_wage = @netWage,
        processed_date = @processedDate
    WHERE employee_id = @employeeID
      AND reference = @reference

    DELETE FROM CarriedForward
    WHERE employee_id = @employeeID

    INSERT INTO CarriedForward (employee_id, carried_forward_amount)
    VALUES(@employeeID, @carriedForwardAmount)

COMMIT;

I assume the business logic is correct. It is unusual to have a table in a database where deletes happen. I am more accustomed to having some sort of history for the data, and the delete is a logical thing, not a real delete. This is normally for audit/reporting purposes.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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