5
\$\begingroup\$

This works well, but can anybody see any faults in this code? If so, could you suggest ways in which I can fix it?

CREATE PROC ValidateUser
    (
      @Email NVARCHAR(100),
      @PasswordHash NVARCHAR(128)
    )
AS 
    IF EXISTS ( SELECT  1
                FROM    dbo.[User]
                WHERE   [Email] = @Email
                        AND PasswordHash = @PasswordHash ) 
        SELECT  'True' 
    ELSE 
        SELECT  'False'
\$\endgroup\$

migrated from stackoverflow.com Jan 13 '12 at 13:50

This question came from our site for professional and enthusiast programmers.

7
\$\begingroup\$

The only "fault" I see is that you return "magic strings" as your result - those are always a bit hard to use, and can cause issues (if you misspell a string).

This is a boolean check - you get back True or False - but I would return the boolean values - not strings:

CREATE PROC ValidateUser
    (@Email NVARCHAR(100),
      @PasswordHash NVARCHAR(128))
AS 
    IF EXISTS (SELECT *
               FROM dbo.[User]
               WHERE [Email] = @Email AND PasswordHash = @PasswordHash) 
        SELECT 1
    ELSE 
        SELECT 0

Or the other option would be to just return some column from the users table - if that user exists and the password hash matches, you get back a value - otherwise you get NULL:

CREATE PROC ValidateUser
    (@Email NVARCHAR(100),
      @PasswordHash NVARCHAR(128))
AS 
    SELECT UserID 
    FROM dbo.[User]
    WHERE [Email] = @Email AND PasswordHash = @PasswordHash) 

So if the user e-mail and password hash match and exist, then you get back the UserID - otherwise you get NULL. That's also a nice, clear, yes-or-no kind of answer

\$\endgroup\$
  • 2
    \$\begingroup\$ Good suggestions! However, in the second query it might be safer to have a TOP 1 clause and to eventually return UserId as an output parameter. \$\endgroup\$ – Cristian Lupascu Jan 13 '12 at 16:09
0
\$\begingroup\$

I would not make this a proc but a function and I woult not return a string. That sort of is all.

Oh, and on top it is simply useless - just pull the hash anyway. There is no logic in there to start with.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.