Plz do evaluate this SP

ALTER PROCEDURE JobsonUser_GetuserListValues (
@.p_PortalID int ,
@.p_UserID int,
@.p_SubscriptionText varchar(255))
AS
BEGIN

SET NOCOUNT ON

Variable Declarations

Declare @.userID int
Declare @.portalID int
Declare @.DynamicQuestionID varchar(255)
Declare @.Question varchar(255)
Declare @.i int
Declare @.count int
Declare @.loop int

Declare @.SubscriptionList Table (PortalID int , Question Varchar(255) , DynamicQuestionID varchar(255))
Declare @.ResponseList Table (DynamicQuestionID varchar(255) , Response varchar(255) , SortOrder int , Pos int IDENTITY (0,1))
Declare @.ResultSet Table (Rid int IDENTITY (0,1),ResultValue smallint)

- Variable Declarations Ends

-- Initializations

Set @.i = 0
Set @.count = 1
Set @.loop = 0

-- Getting User Level Info And Portal ID

-- Step 1

Select
@.userID = a.UserID ,
@.portalID = b.PortalID
From Users a
Left Outer Join UserPortals b
On a.UserId = b.UserID
Where a.UserID = @.p_UserID And b.PortalID = @.p_PortalID

--

- Step 2

Insert into @.SubscriptionList (PortalID,Question , DynamicQuestionID)
Select PortalID,Question,DynamicQuestionID
From DynamicRegistration_Question
Where ShortFieldName = @.p_SubscriptionText

Select Top 1
@.DynamicQuestionID = DynamicQuestionID ,
@.Question = Question
From @.SubscriptionList

--Select @.DynamicQuestionID

select @.Question

Step 3

Insert into @.ResponseList (DynamicQuestionID , Response , SortOrder)
select
a.DynamicQuestionID , b.Response , a.SortOrder
From DynamicRegistration_QuestionOption a
left outer join DynamicRegistration_QuestionResponse b
on a. DynamicQuestionID = b.DynamicQuestionID
And a.QuestionOption = b.Response
Where a.DynamicQuestionID = @.DynamicQuestionID --'B440AC87-FC53-4735-A6D8-EB4DBFD9D447'
And b.UserID = @.userID And a.Inactive = 0
order by a.SortOrder

Select @.count = count(*) From @.ResponseList

IF (@.count > 0 )
Begin
While (@.loop < 3)
Begin
if Exists(Select * from @.ResponseList Where Pos = @.loop)
Begin
Insert into @.ResultSet (ResultValue) values(1)
End
else
Begin
Insert into @.ResultSet (ResultValue) values(0)
End
Set @.count = @.count + 1
Set @.loop = @.loop + 1
End
End
Else
begin
Insert into @.ResultSet (ResultValue) values(0)
Insert into @.ResultSet (ResultValue) values(0)
Insert into @.ResultSet (ResultValue) values(0)
end

Select
case ResultValue When 0 Then 'False'
Else 'True' End as ResultValue
from @.ResultSet
return

ENDMoving to Transact-SQL forum.|||

Arivindt:

First, I give you a lot of credit because you asked to have your code reviewed. This is always worthwhile and I want to encourage you to continue this habbit -- it will serve you well.

The first question that I have involves this line:

Code Snippet

Where a.DynamicQuestionID = @.DynamicQuestionID --'B440AC87-FC53-4735-A6D8-EB4DBFD9D447'

There are some definite performance problems associated with uniqueidentifiers; Is it necessary to use a uniqueidentifier here? Can the table be modified to use a smaller datatype? ( I will assume that this would require too much work ) For future work, avoid UNIQUEIDENTIFIER datatypes as they have a negative impact on performance.

This section of code also bothers me a little:

Code Snippet

Insert into @.SubscriptionList (PortalID,Question , DynamicQuestionID)
Select PortalID,Question,DynamicQuestionID
From DynamicRegistration_Question
Where ShortFieldName = @.p_SubscriptionText

Select Top 1
@.DynamicQuestionID = DynamicQuestionID ,
@.Question = Question
From @.SubscriptionList

The first thing that I noticed was that this was followed by what appears to be a couple of DEBUG statements. Therefore, I presume that this either is giving you some trouble or has given you trouble in the past. It bothers me to see the "TOP 1" clause without an "ORDER BY" clause.

Also, Since the @.SubscriptionList table is not used after this section, I believe that the code can be simplified and perhaps the @.SubscriptionList table variable can be completely eliminated. Something like this might work here:

Code Snippet

select @.DynamicQuestionId = DynamicQuestionId,
@.Question = Question
from ( select top 1
DynamicQuestionId,
Question
from DynamicRegistration_Question
Where ShortFieldName = @.p_SubscriptionText
-- I feel like an order by should be added here.
) x

Note the location to add an ORDER BY clause. I would recommend that you figure out what the ordering should be and add an ORDER BY clause.

I also think that step #3 can be simplified.

|||

Hi,

Thank u so much for evaluating and giving me proper guidence.....

I will do my level best to incorporate the changes and suggestion what i hav e gained from ur

reply.

Once again thanks a lot

Thanks & Regards

Aravind

|||

Aravind:

I think that for step #3 that you might be able to simplify this:

Code Snippet

Insert into @.ResponseList (DynamicQuestionID , Response , SortOrder)
select
a.DynamicQuestionID , b.Response , a.SortOrder
From DynamicRegistration_QuestionOption a
left outer join DynamicRegistration_QuestionResponse b
on a. DynamicQuestionID = b.DynamicQuestionID
And a.QuestionOption = b.Response
Where a.DynamicQuestionID = @.DynamicQuestionID --'B440AC87-FC53-4735-A6D8-EB4DBFD9D447'
And b.UserID = @.userID And a.Inactive = 0
order by a.SortOrder

Select @.count = count(*) From @.ResponseList

IF (@.count > 0 )
Begin
While (@.loop < 3)
Begin
if Exists(Select * from @.ResponseList Where Pos = @.loop)
Begin
Insert into @.ResultSet (ResultValue) values(1)
End
else
Begin
Insert into @.ResultSet (ResultValue) values(0)
End
Set @.count = @.count + 1
Set @.loop = @.loop + 1
End
End
Else
begin
Insert into @.ResultSet (ResultValue) values(0)
Insert into @.ResultSet (ResultValue) values(0)
Insert into @.ResultSet (ResultValue) values(0)
end

Select
case ResultValue When 0 Then 'False'
Else 'True' End as ResultValue
from @.ResultSet
return

END

to something like this:

Code Snippet

select case when y.Seq is not null then 'True' else 'False' end
as ResultValue
from ( select 1 as Seq union all select 2 union all select 3) x
left join ( select row_number() over
( order by a.SortOrder, a.DynamicQuestionId )
as Seq,
a.DynamicQuestionID,
b.Response,
a.SortOrder
From DynamicRegistration_QuestionOption a
left outer join DynamicRegistration_QuestionResponse b
on a. DynamicQuestionID = b.DynamicQuestionID
And a.QuestionOption = b.Response
Where a.DynamicQuestionID = @.DynamicQuestionID --'B440AC87-FC53-4735-A6D8-EB4DBFD9D447'
And b.UserID = @.userID And a.Inactive = 0
order by a.SortOrder
) y
on x.Seq = y.Seq
and y.Seq <= 3

The final thing that I cannot stress enough is to continue to ask for another set of eyes to look over your work! It is for that reason that I really would like for some of the other more senior members of this forum to also add their comments. I frequently miss something and sometimes just flat out get something wrong. My main motivation to joining this forum was to learn and YOUR question today has helped me to continue to learn. I hope you will continue to contribute.

Kent

|||

SELECT
dt.DynamicQuestionID, dt.QuestionOption,
dt.UserID, qo.SortOrder,
CASE
WHEN qr.Response IS NULL THEN 'FALSE'
ELSE 'TRUE'
END AS Status
FROM
(
SELECT
DISTINCT qr.DynamicQuestionID, qr.UserID, qo.QuestionOption FROM
DynamicRegistration_QuestionResponse qr
CROSS JOIN
DynamicRegistration_QuestionOption qo
) AS dt
INNER JOIN
DynamicRegistration_QuestionOption qo
ON
qo.DynamicQuestionID = dt.DynamicQuestionID
AND qo.QuestionOption = dt.QuestionOption
LEFT OUTER JOIN
DynamicRegistration_QuestionResponse qr
ON
dt.DynamicQuestionID = qr.DynamicQuestionID
AND dt.QuestionOption = qr.Response
AND dt.UserID = qr.UserID