Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Aim: To Assess if one, or more, examples of a value exists in a Database in the quickest time as I only needs a True/False result.

The variable is Alphanumeric.

Question: Is this the quickest and best way to do this?

Public Function PrcCheckIfValueExists(vVariable As String) As String
        'Here we check if the Value Exists in the database 
        Try
            Dim ConnectionString As String = System.Configuration.ConfigurationManager.ConnectionStrings("SQLLocal").ToString()
            Using connection = New SqlConnection(ConnectionString)
                Using command As New SqlCommand("SELECT TOP 1 ID FROM TblTable WHERE ID= '" & vVariable & "'", connection)
                    connection.Open()
                    Dim result = command.ExecuteScalar()
                    connection.Close()
                    If result = "" Then
                        Return False
                    Else
                        Return True
                    End If
                End Using
            End Using

        Catch ex As Exception
            Return False
        End Try

    End Function
share|improve this question
3  
    
Shame I can't give both an the answer. –  indofraiser Jun 30 at 15:09
    
Why does the code return Booleans when the signature suggests it should return a string? –  Paul Richards Jul 1 at 7:33
    
Incoming is string outgoing is variable. Should I use As Boolean as last part of the first line? –  indofraiser Jul 1 at 13:07

4 Answers 4

up vote 11 down vote accepted
  1. This code is open to injection. Consider what happens if I pass sometext'; Drop Table TblTable; -- into that variable. Use a proper parameterized query instead.
  2. The SQL statement itself is about as efficient as it can be to my knowledge. A count would add extra overhead. Using an Exists might garner you a negligible amount of performance at the cost of readability. (Exists wouldn't need to return any value at all except what you tell it.)
  3. You could simplify your return statement by using IsNullOrEmpty()

Instead of:

                If result = "" Then
                    Return False
                Else
                    Return True
                End If

You could just use this:

Return Not String.IsNullOrEmpty(result)
share|improve this answer
    
no Semi-Colons in VB...LOL –  Malachi Jun 30 at 15:20
1  
Oops. lol. Thanks @Malachi. –  RubberDuck Jun 30 at 15:21
2  
Also you need to add a Not in there to keep the false return value on empty string. –  Snowbody Jun 30 at 15:31

You should use Command parameters instead of string concatenation to avoid SQL injection attacks

 command.Parameters.Add("@ID", SqlDbType.NVarChar) ' this might not be the appropriate type
 command.Parameters("@ID").Value = vVariable

The connection will be closed after the using statement, so you don't need to close it explicitly

connection.Close() 'delete this line, it is redundant 

vVariable is not good for a variable name, because every variable is a variable, ID might be a good choice here

share|improve this answer
    
I should of mentioned the value derived from clicking on a value however I will change it for good practice, thanks. –  indofraiser Jun 30 at 14:20

In your SQL, use EXISTS instead of TOP 1 ID. It might be faster, but would probably be impossible to measure the difference. But it better expresses what you are interested in. And it certainly won't be slower.

share|improve this answer

The safer way to do this is to actually call a stored procedure.

Separation of Concerns.

This is the line of code that I am talking about

Using command As New SqlCommand("SELECT TOP 1 ID FROM TblTable WHERE ID= '" & vVariable & "'", connection)

The code doesn't need to know how the database query is retrieving the information.

The stored procedure will run just as efficiently or faster, because the database will know about the procedure before it is run and have a "game plan" for it before it is called.

The variable should stay a variable all the way to the stored procedure.


you can remove the connection.Close() and move the if statement outside of the Try Catch like this

Try
    Dim ConnectionString As String = System.Configuration.ConfigurationManager.ConnectionStrings("SQLLocal").ToString()
    Using connection = New SqlConnection(ConnectionString)
        Using command As New SqlCommand("SELECT TOP 1 ID FROM TblTable WHERE ID= '" & vVariable & "'", connection)
            connection.Open()
            Dim result = command.ExecuteScalar()
        End Using
    End Using
Catch ex As Exception
    Return False
End Try
If result = "" Then 
    Return False
Else
    Return True
End If

or you can get rid of the if else and the variable declaration altogether

Try
    Dim ConnectionString As String = System.Configuration.ConfigurationManager.ConnectionStrings("SQLLocal").ToString()
    Using connection = New SqlConnection(ConnectionString)
        connection.Open()
        Using command As New SqlCommand("SELECT TOP 1 ID FROM TblTable WHERE ID= '" & vVariable & "'", connection)
            Return Not String.IsNullOrEmpty(command.ExecuteScalar()) 
        End Using
    End Using
Catch ex As Exception
    Return False
End Try

You will save a little bit when you nix the Variable creation



I somewhat agree with MrCochese.

If the code was returning a list of information with no input parameters needed, then I would say just use the string, but this code is asking for a very specific piece of information and needs a piece of input.

The code(application) gathers the input, sends it to the database. The database looks for the specified piece of information and returns it to the code(application). This shows a separation of concerns (there are two things happening here), input and query.

In the example of returning a simple set of information, only one things is happening, the return of information. There is no input or place for injection to occur.

so I stand by what I said about using a stored procedure, or better yet a user defined function because only one specific piece of information is being returned

share|improve this answer
1  
This is a horrible idea, as you're just introduced an external dependency that is not versioned with your code. Don't do this - let SQL do the job it was created for. –  Mr Cochese Jun 30 at 15:47
1  
The SQL is still doing the SQL stuff, I left the Query String in the code because that is what the OP has there. I mentioned the fact that there should be a stored procedure on the database that is called using a parameter, that still stands. I haven't taken anything away from the database, it's really the opposite I have given back to the database what should rightfully be there @MrCochese –  Malachi Jun 30 at 18:26
1  
@MrCochese If the schema changes in a way that the sproc needs to change, it could be changed without OP recompiling the code. That's why stored procedures are there to begin with. –  RubberDuck Jun 30 at 18:52
1  
Well, it is Code Review! –  Mr Cochese Jun 30 at 20:52
1  
@MrCochese - Then maybe you could post it as a comment under the correct portion, so the OP might have a better chance of being notified/reading it. –  JohnP Jun 30 at 21:18

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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