Adding to an index of authors

I'm new to SQLite but have programmed in BASIC (and other languages) for pushing 50 years.

Given the sample code extract, please offer suggestions for improvement:

Sub addAuthor(ByVal authorlnf As String, ByVal authorfnf As String)
        Dim objConn As SQLiteConnection
        Dim objCommand As SQLiteCommand
        objConn = New SQLiteConnection(CONNECTION_STR)
        objConn.Open()
        objCommand = objConn.CreateCommand()
        objCommand.CommandText = "INSERT INTO [author] ([authorlnf], [authorfnf]) VALUES ('" & authorlnf & "', '" & authorfnf & "');"
        Try
            objCommand.ExecuteNonQuery()
        Catch ex As InvalidOperationException
            MessageBox.Show("An error has occurred: " & ex.Message)
        Catch ex As SQLiteException
            MessageBox.Show("An error has occurred: " & ex.Message)
        End Try

        If Not IsNothing(objConn) Then objConn.Close()

End Sub

This is an index of book authors and this routine will be called several thousand times. I'm replacing VB dictionaries as memory is becoming a limiting factor.

I have never used SQLite either, but something is jumping at me in your code: the addAuthor() procedure owns the connection, which means if you're calling it 10,000 times in a loop you're going to be opening and closing 10,000 connections, which is ..well to be honest, nuts.

Looking at the method's signature:

Sub addAuthor(ByVal authorlnf As String, ByVal authorfnf As String)

Naming nitpicks

• I believe PascalCasing is the convention for naming just about everything in VB, but what really matters is consistency. Try to be consistent with the language you're using - the VB API uses PascalCasing for methods.

• Names authorlnf and authorfnf could just as well be named authorwtf - they tell the reader it's something about an author, but to find out what exactly, one has to dig deeper than the method's signature. Avoid disemvoweling at all costs - it ruins reading even the best-written code base.

well now that I think of it I'd say authorlnf would be author's last name and authorfnf would be author's first name.. still scratching my head as for what the last f is standing for...

Serious stuff

What happens when authorlnf contains the value "Robert'); DROP TABLE author;--"?

credits: xkcd/Randall Munroe

Your insert fails and the author table gets dropped (if your user has the permissions and there's no FK constraint involved, but that's not the point). This is called SQL Injection and, depending on where, how and by whom your code is being used, it may or may not be a concern - nevertheless, it's a flaw in your design.

You could write yourself some parameter-cleansing function and think you're protected, but that would leave you the burden of proving that your cleansing function is undeniably safe. And you must never forget to call it!

Or you could use parameters, that take the value of the user input. That's much harder to mess with!

Connection ownership

I think the AddAuthor() method shouldn't create (/own) the connection it's using. Consider this:

Sub AddAuthor(ByVal connection As SQLiteConnection, ByVal authorlnf As String, ByVal authorfnf As String)

All of a sudden, opening, closing and disposing the connection is no longer a concern here: you put that burden on the caller, where you're looping 10,000 times - so you open up the connection, create and execute 10,000 commands inside AddAuthor(), and then close the connection. That puts the lite in SQLite, no?

I'll only add that you're catching known & relevant exceptions and letting unknown ones bubble up, and that's excellent. The only thing is, as @Malachi correctly pointed out, you should be closing the connection in a Finally block, but if your method doesn't own the connection, that's moot. However I believe a Command would implement IDisposable, so it should be .Dispose()'d.

StackExchange Code Review Q#27184, answer score: 7