refactor the store proc

[deejayAr]

can any one help me to refactor this store proc
it seems to me very ugly


ALTER PROCEDURE [dbo].[SearchTransactions]
@SiteId varchar(4)
, @ProviderId int
, @PatientFirstName varchar(35)
, @PatientLastName varchar(60)
, @UnsentTransactionsOnly bit
, @GEDIPayerId varchar(10)
, @SubscriberId varchar(20)
, @TemplateId int
, @TypeOfDateSearch varchar(20)
, @DateSearchFrom datetime
, @DateSearchTo datetime
, @Debug bit = 0

AS
BEGIN
DECLARE @SQL nvarchar(max)
, @ParamList nvarchar(4000);

--Adds day to properly handle date range logic (ex. 8/12/2011 would become 8/13/2011 for < @DateSearchTo filter)
SET @DateSearchTo = DATEADD(day, 1, @DateSearchTo);


IF @UnsentTransactionsOnly = 1
SET @SQL = 'SELECT q.RTPARequestId
, q.PatientFirstName
, q.PatientLastName
, v.FirstName + '' ''+ v.LastName AS Provider
, q.SubmissionDate
, q.DateOfServiceFrom
, p.PayerName
, t.TemplateName
, q.RTPARequest
, case when q.ProviderId=2 then 1 else 0 end as ProviderIsValid
FROM dbo.RTPARequest q
INNER JOIN dbo.Payer p ON q.GEDIPayerId = p.GEDIPayerId
LEFT JOIN dbo.Provider v ON q.ProviderId = v.ProviderId
LEFT JOIN dbo.RTPAResponse s ON q.RTPARequestId = s.RTPARequestId
LEFT JOIN dbo.Template t ON q.TemplateId = t.TemplateId
WHERE q.SiteId = @SiteId' + CHAR(13) + CHAR(10) ;

ELSE
SET @SQL = 'SELECT q.RTPARequestId
, q.PatientFirstName
, q.PatientLastName
, v.FirstName + '' ''+ v.LastName AS Provider
, q.SubmissionDate
, q.DateOfServiceFrom
, p.PayerName
, t.TemplateName
, q.RTPARequest
, s.RTPAResponse
, case when q.ProviderId=2 then 1 else 0 end as ProviderIsValid
FROM dbo.RTPARequest q
INNER JOIN dbo.Payer p ON q.GEDIPayerId = p.GEDIPayerId
LEFT JOIN dbo.Provider v ON q.ProviderId = v.ProviderId
LEFT JOIN dbo.RTPAResponse s ON q.RTPARequestId = s.RTPARequestId
LEFT JOIN dbo.Template t ON q.TemplateId = t.TemplateId
WHERE q.SiteId = @SiteId' + CHAR(13) + CHAR(10) ;

IF @ProviderId IS NOT NULL
SET @SQL = @SQL + ' AND q.ProviderId = @ProviderId' + CHAR(13) + CHAR(10);

IF @PatientFirstName IS NOT NULL
SET @SQL = @SQL + ' AND q.PatientFirstName = @PatientFirstName' + CHAR(13) + CHAR(10);

IF @PatientLastName IS NOT NULL
SET @SQL = @SQL + ' AND q.PatientLastName = @PatientLastName' + CHAR(13) + CHAR(10);

IF @GEDIPayerId IS NOT NULL
SET @SQL = @SQL + ' AND q.GEDIPayerId = @GEDIPayerId' + CHAR(13) + CHAR(10);

IF @SubscriberId IS NOT NULL
SET @SQL = @SQL + ' AND q.SubscriberId = @SubscriberId' + CHAR(13) + CHAR(10);

IF @TemplateId IS NOT NULL
SET @SQL = @SQL + ' AND q.TemplateId = @TemplateId' + CHAR(13) + CHAR(10);

IF @TypeOfDateSearch = 'Date Of Service'
BEGIN
IF @DateSearchFrom IS NOT NULL
SET @SQL = @SQL + ' AND q.DateOfServiceFrom >= @DateSearchFrom' + CHAR(13) + CHAR(10);
IF @DateSearchTo IS NOT NULL
SET @SQL = @SQL + ' AND q.DateOfServiceFrom < @DateSearchTo' + CHAR(13) + CHAR(10);
END
ELSE
IF @TypeOfDateSearch = 'Date Of Submission'
BEGIN
IF @DateSearchFrom IS NOT NULL
SET @SQL = @SQL + ' AND q.SubmissionDate >= @DateSearchFrom' + CHAR(13) + CHAR(10);
IF @DateSearchTo IS NOT NULL
SET @SQL = @SQL + ' AND q.SubmissionDate < @DateSearchTo' + CHAR(13) + CHAR(10);
END

IF @UnsentTransactionsOnly = 1
SET @SQL = @SQL + ' AND s.RTPARequestId IS NULL' + CHAR(13) + CHAR(10);

IF @Debug = 1
PRINT @SQL;

SET @ParamList = '@SiteId varchar(4),
@ProviderId int,
@PatientFirstName varchar(35),
@PatientLastName varchar(60),
@GEDIPayerId varchar(10),
@SubscriberId varchar(20),
@TemplateId int,
@TypeOfDateSearch varchar(20),
@DateSearchFrom datetime,
@DateSearchTo datetime';

EXEC sp_executesql @SQL, @ParamList
, @SiteId
, @ProviderId
, @PatientFirstName
, @PatientLastName
, @GEDIPayerId
, @SubscriberId
, @TemplateId
, @TypeOfDateSearch
, @DateSearchFrom
, @DateSearchTo;

RETURN @@ERROR ;

END

thanks

 

[gmmastros]

I don't think it's ugly. The only thing I would probably do is remove the "Char(13) + Char(10)" parts because they are not needed. They're not hurting anything either. I would remove them because I think it would make the code easier to read.

Other than that, I really wouldn't do anything.

-George
Microsoft SQL Server MVP
My Blogs
SQLCop
"The great things about standards is that there are so many to choose from." - Fortune Cookie Wisdom

 

[deejayAr]

Actually I'm interested in to add paging and sorting option into this store proc do you have any suggession

thanks

 

[Qik3Coder]

deejayAr,
I haven't had my coffee yet, but why do you have the IF ELSE block? They look identitical.

There are several ways of handling optional parameters, and they don't look bad once you get used to them.

 IF @ProviderId IS NOT NULL             
[tab]SET @SQL = @SQL + ' AND q.ProviderId = @ProviderId' + CHAR(13) + CHAR(10);
[green]--becomes[/green]
AND ((@ProviderID is NULL) or (q.ProviderID = @ProviderID))
[green]--OR[/green]
AND q.ProviderID = Coalesce(@ProviderID, q.ProviderID)

as GMM has highlighted in the past, you have to be careful of values that are null or empty strings, when adding comparisons like this.

Lodlaiden

You've got questions and source code. We want both!

 

[deejayAr]

thank you very much

 

[deejayAr]

Qik3Coder
then how to write this logic in your way

IF @TypeOfDateSearch = 'Date Of Service' BEGIN IF @DateSearchFrom IS NOT NULL SET @SQL = @SQL + ' AND q.DateOfServiceFrom >= @DateSearchFrom' + CHAR(13) + CHAR(10); IF @DateSearchTo IS NOT NULL SET @SQL = @SQL + ' AND q.DateOfServiceFrom < @DateSearchTo' + CHAR(13) + CHAR(10); END ELSE IF @TypeOfDateSearch = 'Date Of Submission' BEGIN IF @DateSearchFrom IS NOT NULL SET @SQL = @SQL + ' AND q.SubmissionDate >= @DateSearchFrom' + CHAR(13) + CHAR(10); IF @DateSearchTo IS NOT NULL SET @SQL = @SQL + ' AND q.SubmissionDate < @DateSearchTo' + CHAR(13) + CHAR(10); END




I appriciate your effort for helping me

 

[markros]

Your code seems to be OK. If you want to introduce paging, you may want to add ROW_NUMBER() function, re-write the above as CTE (dynamic) and add an actual query to select only rows for a page.

PluralSight Learning Library