0

This is my code to insert the image into an Access DB. The problem is when I insert player image at the first time it works. but when I add another player to the DB and choose another image, it takes the first one. How I can clear the MemoryStream using VB.NET 2015.

Dim conn As New OleDbConnection("provider=microsoft.ace.oledb.12.0; Data Source=SoccerTimeDB.accdb")
Dim cmd As New OleDbCommand("", conn)
Dim ms As New MemoryStream
Sub Runcommand(Sqlcommand As String, Optional message As String = "")
    Try
        If conn.State = ConnectionState.Closed Then conn.Open()
        cmd.CommandText = Sqlcommand
        cmd.ExecuteNonQuery()
        If message <> "" Then MsgBox(message)
    Catch ex As Exception
        MsgBox(ex.Message)
    Finally
        If conn.State = ConnectionState.Open Then conn.Close()
    End Try
End Sub

Private Sub btnAdd_Click(sender As Object, e As EventArgs) Handles btnAdd.Click
    playerImage.Image.Save(ms, playerImage.Image.RawFormat)
    Dim img() As Byte
    img = ms.ToArray()
    Dim strCmd As String = "insert into playerData values (" & playerNumberTB.Text & "," & playerIdTB.Text & ",'" & playerNameTB.Text & "', @Img) "

    cmd.Parameters.AddWithValue("@Img", img)
    Runcommand(strCmd, "player has been added")
End Sub
  • 2
    What is stopping you from creating a new memory stream inside the add function? I don't see any reason why you are defining it globally in your module? Ideally you would research using a [`Using Block`](https://learn.microsoft.com/en-us/dotnet/visual-basic/language-reference/statements/using-statement) as you should ideally dispose the memorystream – Icepickle Oct 22 '18 at 10:46
  • The first Save() call leaves the memory stream positioned at the end of the stream. So subsequent calls append to the stream, the first image is still present in the stream. Eventually the program will bomb with OutOfMemoryException. Adding `ms.Position = 0` before the Save() call is required to fix this code. – Hans Passant Oct 24 '18 at 10:00

1 Answers1

2

A few problems with your code:

  • The MemoryStream doesn't have to be declared at the class level if you're only using it in one method.
  • You always need to dispose the stream after you're done with it (either by calling the Dispose() method or preferrably by wrapping it in a Using block).
  • The whole point of using parameterized queries is to prevent SQL injection. Therefore, you need to use parameters for all your values, not just the image.

Try this out:

Dim img() As Byte
Using ms As New MemoryStream()
    playerImage.Image.Save(ms, playerImage.Image.RawFormat)
    img = ms.ToArray()
End Using

Dim strCmd As String = 
    "INSERT INTO playerData VALUES (@PlayerNumber, @PlayerId, @PlayerName, @Img)"
cmd.Parameters.AddWithValue("@PlayerNumber", playerNumberTB.Text)
cmd.Parameters.AddWithValue("@PlayerId", playerIdTB.Text)
cmd.Parameters.AddWithValue("@PlayerName", playerNameTB.Text)
cmd.Parameters.AddWithValue("@Img", img)
Runcommand(strCmd, "player has been added")

Note: You need to remove the Dim ms As New MemoryStream line from your declarations because it's not required anymore.


Update:

The same applies to your OleDbConnection and OleDbCommand, you don't need those in the declaration as well. But if you do, make sure, at least, that you don't reuse the command. Dispose it and re-initialize it or clear its parameters.

I would recommend something like this for your RunCommand method:

Private ConnString As String = 
    "provider=microsoft.ace.oledb.12.0; Data Source=SoccerTimeDB.accdb"

Sub Runcommand(cmdText As String, Optional message As String = "")
    Using conn As New OleDbConnection(ConnString)
        Using cmd As New OleDbCommand(cmdText, conn)
            Try
                conn.Open()
                cmd.ExecuteNonQuery()
                If Not String.IsNullOrEmpty(message) Then MsgBox(message)
            Catch ex As Exception   ' Try to catch specific exceptions instead.
                MsgBox(ex.Message)
            End Try                 ' You don't need the `Finally` block anymore.
        End Using
    End Using
End Sub
  • 2
    You may not be aware of these articles: [AddWithValue is Evil](http://www.dbdelta.com/addwithvalue-is-evil/) and [Can we stop using AddWithValue() already?](https://blogs.msmvps.com/jcoehoorn/blog/2014/05/12/can-we-stop-using-addwithvalue-already/). There are even more articles available explaining why AddWithValue should not be used. – Andrew Morton Oct 22 '18 at 14:07
  • I changed my code to be like yours but the problem is still there. - removed ms declaration. - I tried to dispose stream. - change SQL string as yours. – izadin alathwari Oct 22 '18 at 19:52
  • @AndrewMorton Yep, you're right about that. I just didn't want to change the OP's code logic much considering that these are just simple strings and ints. I still second you on that though. `AddWithValue` is almost always discouraged. The OP can follow the guidelines from these two articles to manually provide the type using `Add()` instead. – 41686d6564 stands w. Palestine Oct 24 '18 at 06:58
  • @izadinalathwari Are you sure that `Image` in `playerImage.Image.Save` belongs to the second player, not the first one? I would check there first. – 41686d6564 stands w. Palestine Oct 24 '18 at 06:59
  • I just noticed that you're reusing the OleDbCommand. I've updated the answer above. – 41686d6564 stands w. Palestine Oct 24 '18 at 07:16