8
\$\begingroup\$

I am learning TSQL (SQL in general for that matter), and I wrote this trigger that inserts logs about updated employees into the tblEmployeeAudit table.

When I was looking at the end result my eyes hurt.
The trigger was super simple, yet super long (for something which is so simple), it seemed as if a 10-year-old rascal wrote it. Does anyone know if I can make this any better/shorter, and/or How to avoid repeating same line of code 100 times? Here's the code.

Alter TRIGGER tr_tblEmployee_ForUpdate
ON tblEmployee
FOR UPDATE

AS
declare @id int
declare @Name nvarchar(50)
declare @GenderID int
declare @Salary int
declare @City nvarchar(50)
declare @DepartmentID int
declare @ManagerID int
declare @DateOfBirth datetime

declare @Nameol nvarchar(50)
declare @GenderIDol int
declare @Salaryol int
declare @Cityol nvarchar(50)
declare @DepartmentIDol int
declare @ManagerIDol int
declare @DateOfBirthol datetime  

BEGIN
    Select @id = employeeid from inserted

    Select @Name = Name 
          ,@GenderID = GenderID
          ,@Salary = Salary
          ,@City = City
          ,@DepartmentID = DepartmentID
          ,@ManagerID = ManagerID
          ,@DateOfBirth = DateOfBirth
    From Inserted

    Select @Nameol = Name 
          ,@GenderIDol = GenderID
          ,@Salaryol = Salary
          ,@Cityol = City
          ,@DepartmentIDol = DepartmentID
          ,@ManagerIDol = ManagerID
          ,@DateOfBirthol = DateOfBirth
    From deleted

    Select * from deleted
    Select * from inserted
    Insert into tblEmployeeAudit2 values(
    'Employee with ID =' 
    + str(@id) 
    + ' changed Name from ' + cast(@Nameol as nvarchar(50)) + ' to ' + cast(@Name as nvarchar(50) )
    + 'Gender from ' +cast(@GenderIDol as nvarchar(5)) +' to ' +cast(@GenderID as nvarchar(5))
    + 'Salary from ' +cast(@SalaryOl as nvarchar(5)) +' to ' +cast(@Salary as nvarchar(5))
    + 'City from ' +cast(@Cityol as nvarchar(50)) +' to ' +cast(@City as nvarchar(50))
    + 'Department from ' +cast(@DepartmentIDol as nvarchar(5)) +' to ' +cast(@DepartmentID as nvarchar(5))
    + 'ManagerID from ' +cast(@ManagerIDol as nvarchar(5)) + ' to ' +cast(@ManagerID as nvarchar(5))
    )

END;
\$\endgroup\$
  • \$\begingroup\$ To avoid repeating code 100's of times, you might find that Change Data Capture meets your needs. \$\endgroup\$ – default.kramer Dec 14 '17 at 18:51
10
\$\begingroup\$

First some comments.

Usage of Deprecated Features

Currently your trigger does a SELECT that returns a resultset from a trigger. That was at one time supported but has been since deprecated since at least 2012. I would remove them.

Audit requirement

I very much agree with @Austin Hasting that it's generally better to not transform the data into a concatenated string here. You lose the ability to query or filter. The user-friendly formatting should be left to the application or at least the stored procedures used by the application. It doesn't belong in the trigger.

Bad assumptions

Your trigger assumes that you will have only one row in inserted and deleted. That is not how T-SQL triggers will work. It works on the statement level, not row level. Therefore you should code your trigger as if it could contain 10 rows in both virtual tables. The way the code is, it may not necessarily error out but it may miss out on other rows because the assignments only operate on the last row being assigned. Please, don't do this row by agonizing row.

Lack of two-part naming

It's a very good habit to always reference to your objects with two-part names, <schema>.<object> and not just <object>, because an object can exist in several schema with same names, and everyone has their default schema which takes precedence over the schema you actually meant to. I'm assuming you are using the default dbo schema. At least qualify that.

Lack of statement terminator

Several features have been added to the T-SQL language to a point where the ; terminator are no longer totally optional. T-SQL has been an aberrant in the SQL standard in making it optional. Some hack their way by putting a ; in front of statement whenever it's required, which is very weird. The best reason to use one everywhere is because it makes for more readable code, and makes your intention explicit.

Lack of block

I really do not like triggers or stored procedure without an explicit BEGIN...END block. It is valid T-SQL to write without one but this has problems and can make for confusing scripting.

Usage of + operator

You are concatenating strings with the + operator. Without knowing the version of your SQL Server, it might be the only choice but I should point out that since 2012, you now have a CONCAT() function which is much better because it will handle the nulls whereas + will propagate nulls and thus leave you with.... a null.

Lack of explicit insert list

When doing an INSERT, and especially when you do have an explicit SELECT list, it's usually good to have an explicit list. Otherwise, schema change could break the query.

With the commentary out of the way, this is how I'd rewrite this.

ALTER TRIGGER tr_tblEmployee_ForUpdate
ON dbo.tblEmployee
FOR UPDATE AS
BEGIN
  WITH Updated AS (
    SELECT
      i.Name,
      i....
      d.Name AS OldName,
      d.... AS Old...
    FROM inserted AS i
    INNER JOIN deleted AS d
      ON i.<primary key> = d.<primary key>
  )
  INSERT INTO dbo.tblEmployeeAudit2 (<Name of column>)
  SELECT CONCAT(
    'Employee with ID = ',
    u.ID,
    ' changed Name from ', u.Name ' to ', u.OldName
    ...
  ) AS AuditDescription
  FROM Updated AS u;
END;
\$\endgroup\$
  • \$\begingroup\$ Truly superb anwser. Thank You so much. By the way i am using MS SQL Server 2017. \$\endgroup\$ – Koteu Dec 14 '17 at 19:27
7
\$\begingroup\$

You are basically reformatting one group of fields into another. I'm not sure if that's a good idea - you might want to just make a straight-up copy of the fields, since that improves your "audit" capabilities - but if you're forced to use that format, then you're forced.

Regardless, see this SO answer for an example of using a nested SELECT in an INSERT statement. You can obviously apply your formatting expressions to convert the field data.

You'll want to do something like:

INSERT INTO tblEmployeeAudit2 
    SELECT 'Employee with ID =' 
           + str(new.id) 
           + ' changed Name from '
           + ...
      FROM inserted AS new, deleted AS old
\$\endgroup\$
  • \$\begingroup\$ Really nice improvement to the code, thank You. I'll see if anyone posts an even better answer until tomorrow. If not, I'll accept your question. \$\endgroup\$ – Koteu Dec 14 '17 at 13:59
  • \$\begingroup\$ I'd watch out on the cartesian join of the tables. If 3 rows were updated, both tables will have 3 rows, which will then generate a result set of 9 rows, which would trip up the auditing. \$\endgroup\$ – this Dec 14 '17 at 17:56
4
\$\begingroup\$

More Comments

While the others have offered great feedback and suggestions, I also noticed a couple things with the current code. The derived table syntax suggested by Austin Hastings will (and likely already has) definitely decrease the length of the trigger. If you still needed to use a transaction similar to the code you posted above, perhaps these comments will be helpful.

Excess SELECT statements

These two SELECT * statements appear to be useless. Perhaps those were left over from debugging.

Select * from deleted
Select * from inserted

Cast to same type?

The name and city local variables have data type nvarchar(50). Those don't need to be cast to nvarchar(50) when used in the INSERT statement

declare @Name nvarchar(50)
declare @City nvarchar(50)
--...
declare @Nameol nvarchar(50)
declare @Cityol nvarchar(50)

--...
Insert into tblEmployeeAudit2 values(
'Employee with ID =' 
+ str(@id) 
+ ' changed Name from ' + cast(@Nameol as nvarchar(50)) + ' to ' + cast(@Name as nvarchar(50) )
\$\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.