Save date to SQL Server table

Question

I am using the following to set a date value to a variable:

    Dim someDate As DateTime = Date.Today.AddDays(1) 

    nextMonday = someDate
    While nextMonday.DayOfWeek <> DayOfWeek.Monday
        nextMonday = nextMonday.AddDays(1)
    End While

I then use the following to insert the dates into a SQL Server table:

   Private Sub CButton1_ClickButtonArea(Sender As Object, e As MouseEventArgs) Handles CButton1.ClickButtonArea
    Dim doenditstring As String = "INSERT INTO Parsversoeke (ma_datum,di_datum)  " & _
                                     "VALUES (" & _
                                       "'" & nextMonday & "'," & _
                                     "'" & nextTuesday & "')" 

    cnn.Open()
    Dim aksie As New SqlClient.SqlCommand(doenditstring, cnn)

    aksie.ExecuteNonQuery()

    cnn.Close()

It works, except I can only do it for two dates. After that I get the following error:

Conversion failed when converting date and/or time from character string

Does anyone have an idea why I get the error?

I don't know much about vb.net (I recognize it), but before executing them could you print them to your console (or log them in some way?). Your dates are best formatted as `YYYY-MM-DD` or `YYYYMMDD` (the ISO 8601 way). — TT., Jan 09 '16 at 09:21
[SQL Injection alert](http://msdn.microsoft.com/en-us/library/ms161953%28v=sql.105%29.aspx) - you should **not** concatenate together your SQL statements - use **parametrized queries** instead to avoid SQL injection. And as a side-effect, this also takes care of those pesky formatting issues (one more or one less single quote etc.) when trying to insert dates in string literal form.... — marc_s, Jan 09 '16 at 10:00

score 6 · Answer 1 · edited May 23 '17 at 12:01

No, this approach of concatenating strings is doomed from the start.
You get syntax errors, conversion errors and possibly Sql Injection Attacks.

Syntax errors: It is not strictly your case, but if a string used as a value contains a single quote then the whole concatenated string becomes syntactically invalid
Conversion errors: You should not worry how to prepare your data to be acceptable as input to your database. What about decimal separators? You should use a point or a comma? It depends on locale settings of the database and if it is on a different culture your code will quickly become a mess filled with useless replace or ToString
Sql Injection: Again, not strictly involved here, but taking anything typed by your users and using it directly as a part of your query is really a great error that could cost a lot to your customers. See the link above

There is only one way to handle this. Parameterized queries

Private Sub CButton1_ClickButtonArea(Sender As Object, e As MouseEventArgs) Handles CButton1.ClickButtonArea
    Dim doenditstring As String = "INSERT INTO Parsversoeke " & _
                                  "(ma_datum,di_datum)  " & _
                                  "VALUES (@m, @t)"

    cnn.Open()
    Dim aksie As New SqlClient.SqlCommand(doenditstring, cnn)
    aksie.Parameters.Add("@m", SqlDbType.Date).Value = nextMonday     
    aksie.Parameters.Add("@t", SqlDbType.Date).Value = nextTuesday 
    aksie.ExecuteNonQuery()
    cnn.Close()
End Sub

In your code, you ask the compiler to convert your DateTime variables to a string and it does the task requested, but it doesn't know that this string will execute an sql command. Sure, you can give it a strong hint using ToString and a format but then you are betting on the database itself to be able to convert that string back to a datatime according to its conversion rules. In the end, why do you want to allow all these conversions? Parameters relieve your code from all this mess.

Notice, now, how your command text is more clear and the job of correctly passing your values is done by the ADO.NET engine itself (and its SqlClient classes) who knows better how to prepare a DateTime variable for your database.

I should also address another problem clearly visible from your code. There is a global connection object that is reused whenever you need it. This is a bad practice because you are never sure of the correct state of this object. What if somewhere, before this code, you hava caught an exception and, as result, your connection is still open? You open it again and you get an new Exception. Again, your program will quickly come to an abrupt end. Moreover these objects contains unmanaged resources that, if not freed, will cause problems every where in your program. I suggest to create a new SqlConnection everytime you need it and be sure to destroy it at the end of your code enclosing it in a Using Statement

Save date to SQL Server table

1 Answers1