SQL query written in a better way

Hi,
Any suggestion on how to make this  query look better.
Thanks in advance


ALTER PROCEDURE  [dbo].[sp123]

(
    
	 @DocIDType int = 0
	
)
AS
BEGIN
	-- SET NOCOUNT ON added to prevent extra result sets from
	-- interfering with SELECT statements.
	SET NOCOUNT ON;  	
	
	declare @SQL varchar(500) = ''
	declare @GroupBy varchar(500) = ''
	
		 SET @SQL = 'SELECT DISTINCT ApprovalDocDisplay ' 

	IF @DocIdtype = 1 
		BEGIN


			
			SET @SQL = @SQL + '   FROM  [vvApp]    ' 
		    SET @SQL = @SQL + ' WHERE  Email = '+ '''' + ''  +''''	
		END


	IF @DocIdtype = 0
		BEGIN
			SET @SQL = @SQL + '   FROM  [vvApp]    ' 
		    SET @SQL = @SQL + ' WHERE  Email <> '+ '''' + ''  +''''	
		END
		  
	SET @SQL = @SQL  + ISNULL(@GroupBy,'')
      
	EXEC(@SQL)


END

Open in new window

RIASAsked:
Who is Participating?
I wear a lot of hats...

"The solutions and answers provided on Experts Exchange have been extremely helpful to me over the last few years. I wear a lot of hats - Developer, Database Administrator, Help Desk, etc., so I know a lot of things but not a lot about one thing. Experts Exchange gives me answers from people who do know a lot about one thing, in a easy to use platform." -Todd S.

Raja Jegan RSQL Server DBA & Architect, EE Solution GuideCommented:
A simple statement like this should do..
FYI, there is no place to pass parameters to your Email column in the original query and hence not available in the code below as well.
	declare @SQL varchar(500) = ''
	declare @GroupBy varchar(500) = ''
	
	SET @SQL = 'SELECT DISTINCT ApprovalDocDisplay FROM [vvApp] WHERE Email ' + CASE @DocIdtype WHEN 1 THEN ' = ' WHEN 0 THEN ' <> ' END + '''' + ''  +'''' + ISNULL(@GroupBy,'')
    EXEC(@SQL)

Open in new window

0
RIASAuthor Commented:
Thanks will try and brb
0
RIASAuthor Commented:
Thanks. But when I try to set a datatabase role in SQL SERVER I get this error:
key cannot be null
Parameter name key:System
0
10 Tips to Protect Your Business from Ransomware

Did you know that ransomware is the most widespread, destructive malware in the world today? It accounts for 39% of all security breaches, with ransomware gangsters projected to make $11.5B in profits from online extortion by 2019.

Raja Jegan RSQL Server DBA & Architect, EE Solution GuideCommented:
Hi RIAS,

Kindly let me know whether the error you have mentioned is for the above procedure or something else..
0
ste5anSenior DeveloperCommented:
See your last thread..

BUT: Is this your entire procedure?? Then why using dynamic SQL at all?

ALTER PROCEDURE [dbo].[sp123] (
    @DocIDType INT = 0
)
AS
    SET NOCOUNT ON;

    SELECT DISTINCT ApprovalDocDisplay
    FROM   [vvApp]
    WHERE  @DocIDType = 1
           AND Email = 'Literal'
           OR @DocIDType = 0
              AND Email <> 'Literal';

Open in new window


Eventually you need an WITH RECOMPILE on large datasets.
0
Mark WillsTopic AdvisorCommented:
Back in a few.... Will type it ups and check it out for you....

Initial thought is why GROUP BY when selecting distinct ?

And how are you passing @Groupby - I can see a declare, but nothing to populate - or is this just a sample extract ?
0
RIASAuthor Commented:
Yes, Mark this a sample extract.
You can correct the query wherever you feel like.
0
ste5anSenior DeveloperCommented:
You can correct the query wherever you feel like.
No, we - well, at lease I - cannot, cause the sample does not show the reason for dynamic SQL. Thus it is a useless exercise.
0
Mark WillsTopic AdvisorCommented:
Well, it seems the only difference for @doctype is either '=' or '<>'

So, first suggestion (not worrying about the groupby - because that can be appended as you currently do) would be :
    declare	 @DocIDType int = 0
	

	declare @SQL varchar(500) = ''
	declare @GroupBy varchar(500) = ''
	
	SET @SQL = 'SELECT DISTINCT ApprovalDocDisplay FROM  [vvApp] where email '+case when @DocIdtype = 1 then '=' else '<>' end + ''''''
	
	print @sql  

Open in new window

But then on closer examination, you are really just checking the length of email. Assuming no index on (or covering) Email, you could just check len(email) to be > 0 or < 1 and maybe incorporate a check for NULL and avoid all those single quotes e.g.
	SET @SQL = 'SELECT DISTINCT ApprovalDocDisplay FROM  [vvApp] where len(isnull(email,space(0))) '+case when @DocIdtype = 1 then '< 1' else '> 0' end 

Open in new window

Now because you do have GROUP BY option then, if it is to be used, then it has to include ApprovalDocDisplay if you are selecting that column (otherwise it will error). So you had best check the variable @Groupby does contain either nothing, or, includes ApprovalDocDisplay as a column....That way you can just mention the extra columns you want in @groupby to group by
    if len(@groupby) > 0 and charindex('ApprovalDocDisplay',@groupby) < 1
	   set @groupby = ' group by '+@groupby+',ApprovalDocDisplay'
	   		  
	SET @SQL = @SQL  + ISNULL(@GroupBy,'')

	print @sql

Open in new window


Thoughts ?
0
Mark WillsTopic AdvisorCommented:
Hi RIAS,

I just had a quick look at your other question (referenced above by ste5an) and just wondering a bit on what problem you are trying to resolve.

It does seem that we might be of more assistance if you could share a bit more about the problem you are trying to resolve....

If it was my code, then there would be a parameter (or look up file) to populate @groupby if it is really needed (after all, you are already selecting DISTINCT ApprovalDocDisplay, so easy to add in columns to select DISTINCT if not doing any aggregations).

And if @groupby isnt needed then would probably make it a table valued function and bypass all the dynamic SQL.

The more code and steps you have using dynamic SQL, the higher the maintenance cost. Something to consider....

Having said that, sometimes dynamic SQL is extremely useful :)

So, if there is more you can share, please do so.

e.g. "But when I try to set a datatabase role in SQL SERVER......."

Cheers,
Mark Wills
0

Experts Exchange Solution brought to you by

Your issues matter to us.

Facing a tech roadblock? Get the help and guidance you need from experienced professionals who care. Ask your question anytime, anywhere, with no hassle.

Start your 7-day free trial
Jim HornMicrosoft SQL Server Developer, Architect, and AuthorCommented:
(1)  Get rid of all of the blank lines unless it divides major sections, e.g. The first seven lines can be rewritten as one line >> ALTER PROCEDURE dbo.sp123 (@DocIDType int = 0)
(2)  Lose the BEGIN and END around the procedure as that's unnecessary
(3)  Keep all lines within a BEGIN and END on the same left indent, unless there are other decision structures.
(4)  Lose all the redundant spaces, i.e. SET @SQL = @SQL + '   FROM  [vvApp]    ' << can be >> SET @SQL = @SQL + ' FROM vvApp'             
(5)  Good Lord add some frEEaking meaningful code comments so the next guy that supports this has a fighting change to figure out what this code is without having do dig through it all.
0
Mark WillsTopic AdvisorCommented:
Back in an hour or two - relocating mobile "office" :)
0
Scott PletcherSenior DBACommented:
ALTER PROCEDURE  [dbo].[sp123]
(
       @DocIDType int = 0
)
AS
SET NOCOUNT ON;        
SELECT DISTINCT ApprovalDocDisplay
FROM [vvApp]
WHERE (@DocIDType = 1 AND Email = '') OR
           (@DocIDType <> 1 AND Email > '')
0
RIASAuthor Commented:
Sorry Experts was away for couple of days.
Thanks for all the help.
0
It's more than this solution.Get answers and train to solve all your tech problems - anytime, anywhere.Try it for free Edge Out The Competitionfor your dream job with proven skills and certifications.Get started today Stand Outas the employee with proven skills.Start learning today for free Move Your Career Forwardwith certification training in the latest technologies.Start your trial today
Microsoft SQL Server 2008

From novice to tech pro — start learning today.