June 10, 2011 at 7:58 am
Perhaps I should have stayed in bed. I woke up for a code review today and one of the first stored procedures I ran into had the following code. The code contained below part of a stored procedure will be executed many times a day perhaps 10's of thousands times a day.
How do you simply this? Can we avoid dynamic SQL or is it just a nature of the beast?
Perhaps I should have hit the snooze button one more time.
--SET NOCOUNT ON added to prevent extra result sets from interfering with SELECT statements.
SET NOCOUNT ON;
DECLARE @ErrSeverity int =16,@ErrMsg nvarchar(4000)
DECLARE @v_QryStr nvarchar(4000)
DECLARE @v_DateCond nvarchar(300)=''
BEGIN TRY
IF @PatientPersonGUID IS NULL OR LEN(@PatientPersonGUID)= 0
RAISERROR ('PatientPersonGUID cannot be NULL/Blank.',@ErrSeverity,1)
IF @TCPersonGUID IS NULL OR LEN(@TCPersonGUID)= 0
RAISERROR ('TCPersonGUID cannot be NULL/Blank.',@ErrSeverity,2)
IF @UserPersonGUID IS NULL OR LEN(@UserPersonGUID)= 0
RAISERROR ('UserPersonGUID cannot be NULL/Blank.',@ErrSeverity,3)
IF LEN(@UserSponsorSiteGUID) = 0 OR @UserSponsorSiteGUID IS NULL
BEGIN
SELECT @UserSponsorSiteGUID = SSUM.SponsorSiteGUID
FROM dbo.SponsorSiteUserMap SSUM
JOIN SponsorSiteDetail SSD ON SSD.SponsorSiteGUID = SSUM.SponsorSiteGUID
WHERE SSUM.CompositePersonGUID = @UserPersonGUID
END
-- Checking the StartDate & EndDate in Input Parameter and creating Date Condition accordingly.
--If StartDate and EndDate both inputs are provided
IF (ISDATE(@StartDate) = 1 AND @StartDate <> '' AND @StartDate IS NOT NULL AND LEN(@StartDate) > 0 AND
ISDATE(@EndDate) = 1 AND @EndDate <> '' AND @EndDate IS NOT NULL AND LEN(@EndDate) > 0)
BEGIN
SET @v_DateCond=' AND PatientEventRecordedDate BETWEEN ''' + @StartDate + ''' AND ''' + @EndDate + ''''
SET @Action = @Action + ' With search criteria StartDate = ' + @StartDate + '; EndDate = ' + @EndDate
END
--If StartDate input is provided and EndDate is NULL.
ELSE IF (ISDATE(@StartDate) = 1 AND @StartDate <> '' AND @StartDate IS NOT NULL AND LEN(@StartDate) > 0 AND
(ISDATE(@EndDate) <> 1 OR @EndDate IS NULL OR LEN(@EndDate) = 0))
BEGIN
SET @v_DateCond=' AND PatientEventRecordedDate BETWEEN ''' + @StartDate + ''' AND ''' + CONVERT(varchar(10),GETDATE(),101) + ''''
SET @Action = @Action + ' With search criteria StartDate = ' + @StartDate
END
--If StartDate is NULL and EndDate input is provided.
ELSE IF ((ISDATE(@StartDate) <> 1 OR @StartDate IS NULL OR LEN(@StartDate) = 0) AND
ISDATE(@EndDate) = 1 AND @EndDate <> '' AND @EndDate IS NOT NULL AND LEN(@EndDate) > 0)
BEGIN
SET @v_DateCond=' AND PatientEventRecordedDate <= ''' + @EndDate + ''''
SET @Action = @Action + ' With search criteria EndDate = ' + @EndDate
END
-- Creating Query String for dynamic Query.
SET @v_QryStr='SELECT EventGUID
,CONVERT(varchar(10),PatientEventRecordedDate,101) AS PatientEventRecordedDate
,CAST(AssessmentTemperature AS varchar(6)) + ''F (''
+ CAST(dbo.svfConvertFarenheitToCelsius(AssessmentTemperature) AS varchar(5)) + ''C) ''
+ AssessmentTemperatureSource AS Temperature
,CAST(AssessmentBPSystolic AS varchar(4)) + ''/''+ CAST(AssessmentBPDiastolic AS varchar(4)) AS AssessmentBloodPressure
,AssessmentRespiration
,AssessmentPulse
,CAST(AssessmentWeight AS varchar(6)) + ''lbs ('' + CAST(dbo.svfConvertLbsToKgs(AssessmentWeight) AS varchar(6)) + ''Kg)'' AS [Weight]
,CAST(AssessmentHeight AS Varchar(6)) + '''''''''' ('' + CAST(dbo.svfConvertInchesToCms(AssessmentHeight) AS varchar(6)) + ''cm)'' AS Height
,dbo.svfCalculateBMI(AssessmentWeight,AssessmentHeight) AS BMI
,SourceShortDescription
,CASEWHEN IsNullified = 1 OR ISNULL(UpdateMask ,'''') = ''Status'' THEN
(SELECT IsExistsHistory FROM dbo.tvfIsExistsHistory(EventGUID))
ELSE
CASE WHEN ParentEventGUID IS NOT NULL THEN
1
ELSE
0
END
END AS IsExistsHistory
,IsNullified
,(CreatedBy.LastName + '', '' + CreatedBy.FirstName) AS CreatedBy
,(Provider.LastName + '', '' + Provider.FirstName) AS Provider
,ClaimNumber
FROM PatientEvents PE
LEFT JOIN Person CreatedBy
ON PE.CreatedByPersonGUID=CreatedBy.CompositePersonGUID
INNER JOIN Person Provider
ON PE.TCPersonGUID=Provider.CompositePersonGUID
WHERE PE.CompositePersonGUID= ''' + CAST(@PatientPersonGUID AS nvarchar(128)) + '''
AND PE.EventType=''Vital''
AND PE.PatientEventStatus=''Active'''
SET @v_QryStr=@v_QryStr + @v_DateCond + ' ORDER BY PatientEventRecordedDate DESC'
-- Executing the Dynamic Query.
Exec sp_executesql @v_QryStr
END TRY
BEGIN CATCH
-- Raise an error with the details of the exception
SELECT @ErrMsg = ERROR_MESSAGE()
RAISERROR(@ErrMsg, @ErrSeverity, 1)
END CATCH
John Zacharkan
June 10, 2011 at 8:06 am
I don't see any reason for this to be dynamic at all.
My first thought was complex search querying, but it seems to be just two input parameters (start and end date).
A simple IsNull or Coalesce statement to clean up the inputs, and the inputs defined as DateTime or Date datatypes, will make about half of this code unnecessary and will definitely eliminate the need for dynamic SQL.
Are the date inputs defined as Date/DateTime, or are they strings?
- Gus "GSquared", RSVP, OODA, MAP, NMVP, FAQ, SAT, SQL, DNA, RNA, UOI, IOU, AM, PM, AD, BC, BCE, USA, UN, CF, ROFL, LOL, ETC
Property of The Thread
"Nobody knows the age of the human race, but everyone agrees it's old enough to know better." - Anon
June 10, 2011 at 8:20 am
GSquared (6/10/2011)
I don't see any reason for this to be dynamic at all.
What he said -
I've not often seen a valid reason for dynamic sql. If yours is being executed that much...poor optimiser and plan handler...
Would be helpful if you printed out the various combinations for review instead of us parsing it.;-)
Cheers,CrispinI can't die, there are too many people who still have to meet me!It's not a bug, SQL just misunderstood me!
June 10, 2011 at 8:31 am
GSquared (6/10/2011)
I don't see any reason for this to be dynamic at all.My first thought was complex search querying, but it seems to be just two input parameters (start and end date).
A simple IsNull or Coalesce statement to clean up the inputs, and the inputs defined as DateTime or Date datatypes, will make about half of this code unnecessary and will definitely eliminate the need for dynamic SQL.
Are the date inputs defined as Date/DateTime, or are they strings?
They are declared as input variables
,@StartDate varchar(10) = NULL
,@EndDate varchar(10) = NULL
John Zacharkan
June 10, 2011 at 9:02 am
Crispin Proctor (6/10/2011)
GSquared (6/10/2011)
I don't see any reason for this to be dynamic at all.What he said -
I've not often seen a valid reason for dynamic sql. If yours is being executed that much...poor optimiser and plan handler...
Would be helpful if you printed out the various combinations for review instead of us parsing it.;-)
My apologies sir, my focus was on how this should be cleaned up. I'll be fighting with offshore for a rewrite while the PM and Dev lead here will argue deadlines, I can see the arguments already that they can go back and clean this up, and I'll be the bad guy for holding the project up because of bad coding. Sorry for the rant.
John Zacharkan
June 10, 2011 at 9:10 am
John, Consider asking for a change to a typed input parameter. A lot of the validation logic will then vanish (Edit: as was previously stated, wasn't trying to be a parrot here) and further to that, regarding the validation logic, ISDATE is flawed (or just quirky depending on how you look at it) in more ways than is worth running down in a forum setting but here is a quick example:
-- returns 0
SELECT ISDATE('00010101')
-- successfully returns date 0001-01-01
SELECT CAST ('00010101' AS DATE)
-- throws out of range error
SELECT CAST ('00010101' AS DATETIME)
-- successfully returns date 0001-01-01 00:00:00.0000000
SELECT CAST ('00010101' AS DATETIME2)
There are no special teachers of virtue, because virtue is taught by the whole community.
--Plato
June 10, 2011 at 1:56 pm
I see this and I start to cry
SET @v_DateCond=' AND PatientEventRecordedDate BETWEEN ''' + @StartDate +
''' AND ''' + @EndDate + ''''
SET @Action = @Action + ' With search criteria StartDate = ' + @StartDate +
'; EndDate = ' + @EndDate
Especially considering that @StartDate and @Enddate are varchar data types...
As for when to use dynamic SQL... multi-option search queries... If a parameter isn't used for the search, pass it in as NULL. But parameterize it all the way in... don't be appending parameters to your SQL query like above.
Great article on it.
http://sqlinthewild.co.za/index.php/2009/03/19/catch-all-queries/
June 10, 2011 at 2:18 pm
It would look something like this:
-- Input Parameters faked for script
DECLARE @StartDate CHAR(10),
@EndDate CHAR(10),
@PatientPersonGUID VARCHAR(25),
@TCPersonGUID VARCHAR(25),
@UserPersonGUID VARCHAR(25),
@UserSponsorSiteGUID VARCHAR(25) ;
--SET NOCOUNT ON added to prevent extra result sets from interfering with SELECT statements.
SET NOCOUNT ON ;
DECLARE @ErrSeverity INT = 16,
@ErrMsg NVARCHAR(4000),
@SDate DATE,
@EDate DATE ;
BEGIN TRY
IF @PatientPersonGUID IS NULL
OR LEN(@PatientPersonGUID) = 0
RAISERROR ('PatientPersonGUID cannot be NULL/Blank.',@ErrSeverity,1)
IF @TCPersonGUID IS NULL
OR LEN(@TCPersonGUID) = 0
RAISERROR ('TCPersonGUID cannot be NULL/Blank.',@ErrSeverity,2)
IF @UserPersonGUID IS NULL
OR LEN(@UserPersonGUID) = 0
RAISERROR ('UserPersonGUID cannot be NULL/Blank.',@ErrSeverity,3)
IF LEN(@UserSponsorSiteGUID) = 0
OR @UserSponsorSiteGUID IS NULL
BEGIN
SELECT @UserSponsorSiteGUID = SSUM.SponsorSiteGUID
FROM dbo.SponsorSiteUserMap SSUM
JOIN SponsorSiteDetail SSD
ON SSD.SponsorSiteGUID = SSUM.SponsorSiteGUID
WHERE SSUM.CompositePersonGUID = @UserPersonGUID
END
-- Checking the StartDate & EndDate in Input Parameter
SET @SDate = ISNULL(@StartDate, 0) ;
SET @EDate = ISNULL(@EndDate, GETDATE()) ;
-- Creating Query String for dynamic Query.
SELECT EventGUID,
CONVERT(VARCHAR(10), PatientEventRecordedDate, 101) AS PatientEventRecordedDate,
CAST(AssessmentTemperature AS VARCHAR(6)) + 'F ('
+ CAST(dbo.svfConvertFarenheitToCelsius(AssessmentTemperature) AS VARCHAR(5))
+ 'C) ' + AssessmentTemperatureSource AS Temperature,
CAST(AssessmentBPSystolic AS VARCHAR(4)) + '/'
+ CAST(AssessmentBPDiastolic AS VARCHAR(4)) AS AssessmentBloodPressure,
AssessmentRespiration,
AssessmentPulse,
CAST(AssessmentWeight AS VARCHAR(6)) + 'lbs ('
+ CAST(dbo.svfConvertLbsToKgs(AssessmentWeight) AS VARCHAR(6))
+ 'Kg)' AS [Weight],
CAST(AssessmentHeight AS VARCHAR(6)) + ''' ('
+ CAST(dbo.svfConvertInchesToCms(AssessmentHeight) AS VARCHAR(6))
+ 'cm)' AS Height,
dbo.svfCalculateBMI(AssessmentWeight, AssessmentHeight) AS BMI,
SourceShortDescription,
CASE WHEN IsNullified = 1
OR UpdateMask = 'Status'
THEN (SELECT IsExistsHistory
FROM dbo.tvfIsExistsHistory(EventGUID))
ELSE CASE WHEN ParentEventGUID IS NOT NULL THEN 1
ELSE 0
END
END AS IsExistsHistory,
IsNullified,
(CreatedBy.LastName + ', ' + CreatedBy.FirstName) AS CreatedBy,
(Provider.LastName + ', ' + Provider.FirstName) AS Provider,
ClaimNumber
FROM PatientEvents PE
LEFT JOIN Person CreatedBy
ON PE.CreatedByPersonGUID = CreatedBy.CompositePersonGUID
INNER JOIN Person Provider
ON PE.TCPersonGUID = Provider.CompositePersonGUID
WHERE PE.CompositePersonGUID = @PatientPersonGUID
AND PE.EventType = 'Vital'
AND PE.PatientEventStatus = 'Active'
AND PatientEventRecordedDate >= @SDate
AND PatientEventRecordedDate <= @EDate
ORDER BY PatientEventRecordedDate DESC
END TRY
BEGIN CATCH
-- Raise an error with the details of the exception
SELECT @ErrMsg = ERROR_MESSAGE()
RAISERROR(@ErrMsg, @ErrSeverity, 1)
END CATCH
You could add more error handling to the SET statements for @SDate and @EDate if you expect non-date values to be assigned to those parameters.
- Gus "GSquared", RSVP, OODA, MAP, NMVP, FAQ, SAT, SQL, DNA, RNA, UOI, IOU, AM, PM, AD, BC, BCE, USA, UN, CF, ROFL, LOL, ETC
Property of The Thread
"Nobody knows the age of the human race, but everyone agrees it's old enough to know better." - Anon
June 10, 2011 at 2:42 pm
Many thanks should go to GSquared for taking the time to make that code sane... I just have to put in that I find the original code implementation breathtakingly bad. :w00t:
--SJT--
"Back slowly away from the database and let the professionals take over." - Anonymous
June 10, 2011 at 11:33 pm
CELKO (6/10/2011)
[sic] functins cannto be optimized.
Actually, they can. Lookup "Inline Table Valued Functions" (iTVF) and Cross Apply in Books Online. Then make the realization that iTVF's can be made to emulate scalar functions by returning a single "row".
Depressing, isn't it?
What's really depressing is that you continue to bad mouth other people and then you make rookie mistakes like the following...
FOREIGN KEY (interviewer_last_name, interviewer_first_name)
REFERENCES Personnel (emp_last_name, emp_first_name),
FOREIGN KEY (provider_last_name, provider_first_name)
REFERENCES Personnel (emp_last_name, emp_first_name),
Better pray there aren't actually 2 John Smiths working in the same place and that no one ever changes their name just because of things like getting married. 😉
--Jeff Moden
Change is inevitable... Change for the better is not.
June 15, 2011 at 2:06 pm
CELKO (6/10/2011)
This is awful non-SQL. This is what OO programmers do to fake pointer chains, and not learn SQL. I would throw the whole damn thing out.
Thank you for taking your time to provide feedback, it was insightful. I can only imagine how frustrating it must be to a person of your expertise to look at such poorly written spaghetti code. The real sad part is someone actually paid for this.
Thank you again.
John Zacharkan
June 15, 2011 at 2:14 pm
GSquared (6/10/2011)
It would look something like this:
--You could add more error handling to the SET statements for @SDate and @EDate if you expect non-date values to be assigned to those parameters.
Thanks for your reply - They're doulble using the field (not sure why) so they will have to interrogate whether this is a date or not. I reviewed what the findings with the developer manager and he's going to put some pressure on getting some more T-SQL expertise over there. I used your example as well as a better approach to what they provided. Again my appreciation for your time.
John Zacharkan
Viewing 12 posts - 1 through 11 (of 11 total)
You must be logged in to reply to this topic. Login to reply