VB.NET | SQL Code Cleanup

Question

I am trying to teach myself a new language in three weeks. Can you please give me some advice on how to clean my code and make it faster or more reliable? This works fine. I don't "need" any 'free' code, I need the knowledge of what I am doing wrong, and what I am doing right so I can learn.

   Using sqlCon = New SqlConnection("Data Source=(LocalDB)\v11.0;AttachDbFilename=|DataDirectory|\ITCSDatabase.mdf;Integrated Security=True")
        Dim resultVar As String
        sqlCon.Open()
        Dim sqlText = "SELECT appLink FROM appTable WHERE Id = @name"
        Dim cmd = New SqlCommand(sqlText, sqlCon)
        cmd.Parameters.AddWithValue("@name", x)
        resultVar = cmd.ExecuteScalar()
        cmd.ToString()
        If y = 1 Then
            System.Diagnostics.Process.Start(resultVar)
        ElseIf y = 0 Then
            System.Diagnostics.Process.Start("iexplore.exe", resultVar)
        Else
            MsgBox("When calling the appCall Function, an invalid parameter is being sent in.")
        End If
        Dim sqlText2 = "UPDATE appTable SET appClickCount = appClickCount + 1 " & _
        "WHERE Id = @x"
        Dim cmd2 = New SqlCommand(sqlText2, sqlCon)
        cmd2.Parameters.AddWithValue("@x", x)
        cmd2.ExecuteNonQuery()
End Using

Answer 1

As you're beginner, your code can be considered ok.

However you may want to consider following points which help you to write reliable (maintainable) code and improve performance.

• You want to store connectionstring to some common place like web.config. You don't have to find and change it all over the place you used it, just change at one place and it will be reflected.
• You may want to use stored procedure instead of inline query. With stored procedure, your sql code will be more maintainable and you can go through execution plan of sql code to find out any performance bottleneck. To change sql code you don't have to modify and compile your .net code.

I hope it helps!

Answer 2

I would suggest following improvements:

• ConnectionString

Store a connectionstring in the web.config-file and retrieve it when needed:

Dim cs as String = ConfigurationManager.ConnectionStrings("WebConfigConnectionStringName").ConnectionString
Using sqlCon = New SqlConnection(cs)
    'Rest of the code

• Stored Procedures

As @Nil suggested you should use Stored Procedures for your queries instead of hard-coded queries. The reasons he provided are what I'd also suggest.

• Exception Handling

Whenever you are performing operations that might throw an exception (SQL operations, File handling, Service calls, ...), it's more than recommended to implement Exception Handling. This way you can catch the exception and let your program handle it instead of letting the exception crash your program:

Try
    'Some operation that might throw an exception
    '...[Code here]

    'Following method will not be executed if previous code causes an Exception
    AnotherOperation()
Catch Ex As Exception
    'Handle the exception (show a message to the user that something went wrong)
Finally
    'This will always execute, even if an exception is thrown
    'Cleanup resources here (SqlConnection, FileReader, ...)
End Try

Further information/reading:

1. How to: Read Connection Strings from the Web.config File
2. Stored Procedures
3. Introduction to Exception Handling in Visual Basic .NET

Hope this helps! :)

Answer 3

Apart from storing the connection strings into config, using stored procedures and exception handling, I also suggest you to think about code re-usability:

In programming world, readable, maintainable and re-usable codes are most welcome. I just wrote a quick example of extract the query to a generic method for future re-use for your case:

Imports System.Data.SqlClient
Imports System.ComponentModel
Imports System.Runtime.CompilerServices

Module SqlMod
    ''' <summary>
    ''' A simple sample of generic adhoc query
    ''' </summary>
    ''' <typeparam name="T"></typeparam>
    ''' <param name="con"></param>
    ''' <param name="cmdTxt"></param>
    ''' <param name="cmdType"></param>
    ''' <param name="converter"></param>
    ''' <param name="sqlParams"></param>
    ''' <returns></returns>
    ''' <remarks></remarks>
    Public Function ExcecuteAdhocQuery(Of T)(con As SqlConnection,
                                             cmdTxt As String,
                                             cmdType As CommandType,
                                             converter As Func(Of SqlDataReader, T),
                                             ParamArray sqlParams As SqlParameter()) As IEnumerable(Of T)
        Dim output As New List(Of T)

        Try
            Using cmd As New SqlCommand(cmdTxt, con) With {.CommandText = cmdType}
                cmd.Parameters.AddRange(sqlParams)

                ' check connection? necessary?
                If con.State <> ConnectionState.Open Then con.Open()

                Dim rdr = cmd.ExecuteReader()
                While rdr.Read()
                    output.Add(converter.Invoke(rdr))
                End While
            End Using
        Catch ex As Exception
            ' error handling?
            ' logging?
            Throw ex
        End Try

        Return output
    End Function

End Module

You can see this is not perfect, but at least extracts non-business related function out as a standalone and re-usable function, where suits most of reading-query needs. and this will leave your business function much simpler and more readable.

and even more, you could build up a unit test case around this simple function and ensure it's reliable! and believe it's neat and readable too.

anyway, it's just a quick untested sample, feel free to tweak it :)