Link to home
Start Free TrialLog in
Avatar of RIAS
RIASFlag for United Kingdom of Great Britain and Northern Ireland

asked on

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

Avatar of Raja Jegan R
Raja Jegan R
Flag of India image

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

Avatar of RIAS

ASKER

Thanks will try and brb
Avatar of RIAS

ASKER

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
Hi RIAS,

Kindly let me know whether the error you have mentioned is for the above procedure or something else..
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.
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 ?
Avatar of RIAS

ASKER

Yes, Mark this a sample extract.
You can correct the query wherever you feel like.
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.
SOLUTION
Avatar of Mark Wills
Mark Wills
Flag of Australia image

Link to home
membership
This solution is only available to members.
To access this solution, you must be a member of Experts Exchange.
Start Free Trial
ASKER CERTIFIED SOLUTION
Link to home
membership
This solution is only available to members.
To access this solution, you must be a member of Experts Exchange.
Start Free Trial
(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.
Back in an hour or two - relocating mobile "office" :)
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 > '')
Avatar of RIAS

ASKER

Sorry Experts was away for couple of days.
Thanks for all the help.