patternsqlMinor
Insert or update user data
Viewed 0 times
userdatainsertupdate
Problem
I inherited an application that uses some stored procedures. Here is a sample of one of the stored procedures used to insert or modify user data.
would like to get the group's opinion on the code. The SQL Server version is Microsoft SQL Server 2014 - 12.0.4100.1 (X64).
```
USE [MyDataBase]
GO
/ Object: StoredProcedure [dbo].[InsertOrModifyUserData] Script Date: 2/2/2016 8:39:57 PM /
SET ANSI_NULLS ON
GO
SET QUOTED_IDENTIFIER ON
GO
ALTER PROCEDURE [dbo].[InsertOrModifyUserData]
@userName NVARCHAR(200),
@isSuperAdmin BIT = NULL,
@modifiedBy NVARCHAR(200),
@modifiedDate DATETIME = NULL,
@isActive BIT = NULL
AS
BEGIN
SET NOCOUNT ON;
SET XACT_ABORT ON;
BEGIN TRY
BEGIN TRANSACTION
IF (@modifiedDate IS NULL OR @modifiedDate ='1900-01-01 00:00:00.000')
SET @modifiedDate = GETDATE()
IF (@isActive IS NULL)
SET @isActive = 1
IF ((@userName IS NOT NULL OR @userName <>'') AND (@isSuperAdmin IS NOT NULL OR @isSuperAdmin<>'') AND (@modifiedBy IS NOT NULL OR @modifiedBy <>''))
BEGIN
IF EXISTS(SELECT UserName FROM [dbo].[UserAccess] WHERE UserName = @userName)
UPDATE [dbo].[UserAccess]
SET IsSuperAdmin = @isSuperAdmin,
ModifiedBy = @modifiedBy,
ModifiedDate = @modifiedDate,
IsActive = @isActive
WHERE UserName = @userName
ELSE
INSERT INTO [dbo].[UserAccess] (UserName, IsSuperAdmin, CreatedDate, ModifiedBy, ModifiedDate,IsActive)
VALUES(@userName, @isSuperAdmin, GETDATE(), @modifiedBy, @modifiedDate, @isActive)
END
ELSE
RAISERROR('Required parameters are not provided or Required parameters are passed as NULL',13,1)
COMMIT TRANSACTION
select null
END TRY
would like to get the group's opinion on the code. The SQL Server version is Microsoft SQL Server 2014 - 12.0.4100.1 (X64).
```
USE [MyDataBase]
GO
/ Object: StoredProcedure [dbo].[InsertOrModifyUserData] Script Date: 2/2/2016 8:39:57 PM /
SET ANSI_NULLS ON
GO
SET QUOTED_IDENTIFIER ON
GO
ALTER PROCEDURE [dbo].[InsertOrModifyUserData]
@userName NVARCHAR(200),
@isSuperAdmin BIT = NULL,
@modifiedBy NVARCHAR(200),
@modifiedDate DATETIME = NULL,
@isActive BIT = NULL
AS
BEGIN
SET NOCOUNT ON;
SET XACT_ABORT ON;
BEGIN TRY
BEGIN TRANSACTION
IF (@modifiedDate IS NULL OR @modifiedDate ='1900-01-01 00:00:00.000')
SET @modifiedDate = GETDATE()
IF (@isActive IS NULL)
SET @isActive = 1
IF ((@userName IS NOT NULL OR @userName <>'') AND (@isSuperAdmin IS NOT NULL OR @isSuperAdmin<>'') AND (@modifiedBy IS NOT NULL OR @modifiedBy <>''))
BEGIN
IF EXISTS(SELECT UserName FROM [dbo].[UserAccess] WHERE UserName = @userName)
UPDATE [dbo].[UserAccess]
SET IsSuperAdmin = @isSuperAdmin,
ModifiedBy = @modifiedBy,
ModifiedDate = @modifiedDate,
IsActive = @isActive
WHERE UserName = @userName
ELSE
INSERT INTO [dbo].[UserAccess] (UserName, IsSuperAdmin, CreatedDate, ModifiedBy, ModifiedDate,IsActive)
VALUES(@userName, @isSuperAdmin, GETDATE(), @modifiedBy, @modifiedDate, @isActive)
END
ELSE
RAISERROR('Required parameters are not provided or Required parameters are passed as NULL',13,1)
COMMIT TRANSACTION
select null
END TRY
Solution
Dead code
In this:
This line of code:
Errors not useful
You could improve this error reporting:
TLDR: Don't filter error logs, just store stuff. Developers can filter them easily if they need to.
Why? As it is, this is raising an error with no information on "why" the error was raised. At the very least return the input parameters so the user/dev can decipher what is wrong. This is especially relevant if you are logging those errors somewhere.
You would want to include as much relevant information as possible in your errors, especially if there are multiple instances and databases involved. There are few things which hinder improvement and bug fixes more than half-arsed error logs. Log everything and let the developers sort it out.
In this:
COMMIT TRANSACTION
select null
END TRYThis line of code:
select null does nothing useful (read: nothing at all) and should be deleted. I have no idea why it is there to begin with, but querying for NULL with no criteria will always be null.Errors not useful
You could improve this error reporting:
ELSE
RAISERROR('Required parameters are not provided or Required parameters are passed as NULL',13,1)TLDR: Don't filter error logs, just store stuff. Developers can filter them easily if they need to.
Why? As it is, this is raising an error with no information on "why" the error was raised. At the very least return the input parameters so the user/dev can decipher what is wrong. This is especially relevant if you are logging those errors somewhere.
You would want to include as much relevant information as possible in your errors, especially if there are multiple instances and databases involved. There are few things which hinder improvement and bug fixes more than half-arsed error logs. Log everything and let the developers sort it out.
Code Snippets
COMMIT TRANSACTION
select null
END TRYELSE
RAISERROR('Required parameters are not provided or Required parameters are passed as NULL',13,1)Context
StackExchange Code Review Q#118712, answer score: 3
Revisions (0)
No revisions yet.