Data Reader

[dr225]

Hi,


I receive the following error message at run time:

There is already an open DataReader associated with this Command which must be closed first.

Dim con As New SqlConnection("Data Source=DESKTOP-O6TSDMJ;Initial Catalog=RCMS;Integrated Security=True")

        Try
            DsUnitdetails.EnforceConstraints = False

            'TODO: This line of code loads data into the 'DsUnitdetails.vwunitdetails' table. You can move, or remove it, as needed.
            Me.VwunitdetailsTableAdapter.Fill(Me.DsUnitdetails.vwunitdetails, TxtPropertyIDClick.Text)

            'Upload data the combobox from data view in SQL

            con.Open()
            For i As Integer = 0 To DgUnitsUp.Rows.Count - 1
                Dim cmd As New SqlCommand("Select flattype from lkupflattype", con)
                Dim myreader As SqlClient.SqlDataReader = cmd.ExecuteReader
                FlatType.Items.Clear()
                While myreader.Read
                    FlatType.Items.Add(myreader("FlatType"))
                End While
            Next


            con.Close()

            DgUnitsUp.ClearSelection()

        Catch ex As Exception
            MessageBox.Show(ex.ToString())
        End Try

What am I doing wrong?


Thanks

[jmcilhinney]

Isn't it obvious? You're being told that you have an open data reader and you have to close it. Look at the code you posted:

Dim myreader As SqlClient.SqlDataReader = cmd.ExecuteReader

That line opens a data reader. Where are you closing it? You aren't, so why would you be surprised that you get that error message when you try to open another one? This is a perfect example of why you should ALWAYS create objects that support it with an Using statement. The object will then be disposed at the end of the block. Disposed means closed for objects that support closing, like database connections and data readers. Yes, you should be creating your connection with a Using statement as well.

That rather obvious mistake aside, what exactly is up with that code? Why is this code:

                Dim cmd As New SqlCommand("Select flattype from lkupflattype", con)
                Dim myreader As SqlClient.SqlDataReader = cmd.ExecuteReader
                FlatType.Items.Clear()
                While myreader.Read
                    FlatType.Items.Add(myreader("FlatType"))
                End While

inside this loop:

            For i As Integer = 0 To DgUnitsUp.Rows.Count - 1
                '...
            Next

when the loop counter isn't being used at all? You're just executing the same code over and over again, once for each row in what appears to be a DataGrid. Why would you do that? Maybe that explains why you were confused by that error message, i.e. you only ever intended to open one data reader. If that's the case, why are you doing it in a loop in the first place? Even more importantly, why didn't you debug the code properly, which would have made it blatantly obvious that you were doing the wrong thing? This question should never have been asked in the first place because you should have debugged your code for yourself before even considering posting a question here. Maybe you would have still needed to ask a different question, but certainly not this one.

[dr225]

Thanks jmcilhinney - Always very helpful and to the point!

[dday9]

My guess is that you forgot to dispose of a previously created instance of the DataReader. You can clean this up a bit and not worry about disposals by wrapping your declarations in Using statements:

Try
    Using con As New SqlConnection("Data Source=DESKTOP-O6TSDMJ;Initial Catalog=RCMS;Integrated Security=True")
        DsUnitdetails.EnforceConstraints = False
        Me.VwunitdetailsTableAdapter.Fill(Me.DsUnitdetails.vwunitdetails, TxtPropertyIDClick.Text)

        con.Open()
        For i As Integer = 0 To DgUnitsUp.Rows.Count - 1
            Using cmd As New SqlCommand("Select flattype from lkupflattype", con)
                Using myreader As SqlClient.SqlDataReader = cmd.ExecuteReader
                    FlatType.Items.Clear()
                    While myreader.Read
                        FlatType.Items.Add(myreader("FlatType"))
                    End While
                End Using
            End Using
        Next
    End Using
Catch ex As Exception
    MessageBox.Show(ex.ToString())
End Try

I do want to mention that you probably have a flaw in your business logic. Right now you're looping over the DataGridView Rows and running the same command every time. If you have 100 rows, do you really want to get every flattype from lkupflattype, clear the FlatType control, and add the results back 100 times? To me it seems like you'd only want to do it once and so I'd recommend ditching the For/Next loop.

Edit - I took too long to type

[jmcilhinney]

Actually, looking more closely at that code, it makes even less sense. You obviously have a typed DataSet because you're calling Fill on a table adapter just before that code, so why are you using a data reader at all? You should just be filling Me.DsUnitdetails.lkupflattype using the appropriate table adapter and then binding that DataTable to whatever FlatType is, which I suspect is a ComboBox or the like. If you don't want to do that because you don't want to unnecessarily pull back other columns that you're not using then you should be adding a new table adapter and DataTable to your typed DataSet that only includes the columns you actually want. If you're going to use a typed DataSet then use it. Don't half use it and not other times just because you haven't taken the time to learn how to use the DataSet designer.

[dr225]

Thanks dday5, perfect!

Using con As New SqlConnection("Data Source=DESKTOP-O6TSDMJ;Initial Catalog=RCMS;Integrated Security=True")
                DsUnitdetails.EnforceConstraints = False
                Me.VwunitdetailsTableAdapter.Fill(Me.DsUnitdetails.vwunitdetails, TxtPropertyIDClick.Text)

                con.Open()
                'For i As Integer = 0 To DgUnitsUp.Rows.Count - 1
                Using cmd As New SqlCommand("Select flattype from lkupflattype", con)
                        Using myreader As SqlClient.SqlDataReader = cmd.ExecuteReader
                            FlatType.Items.Clear()
                            While myreader.Read
                                FlatType.Items.Add(myreader("FlatType"))
                            End While
                        End Using
                    End Using
                'Next
            End Using
        Catch ex As Exception
            MessageBox.Show(ex.ToString())

Now, the logic I had was I have a textbox column on the datagridview called 'DgUnitsUp' header text = FlatType - Cell (5). Now the values are populated from binding data. It works fine.

So the next step is, if a USER options to EDIT the FlatType, then I would play with the controls and have the textbox control set to False and combobox set to True.

Note: the FlatType combobox is stand alone on the datagridview and gets all the FlatTypes from a lookup table E.g., of flattypes include; one bedroom, two bedroom, three bed room etc.

I hope the logic is acceptable.

On the above code when can I add something to say -

the displayed value of the FlatType on the textbox should be the SelectedValue for the Combobox on the datagridview?

The User then has an option to change and I'll update that to the database accordingly.

Thanks

[dday9]

You can read the data into a data adapter, fill a data table with the data from the adapter, and then bind the data table to the combobox. This process should only happen once (unless you expect the lookup values to change), presumably during the Form's Load event.

Dim dt As New DataTable()
Using cmd As New SqlCommand("Select flattype from lkupflattype", con)
    Using adapter As New SqlDataAdapter(cmd)
        adapter.Fill(dt)
        FlatType.DataSource = dt
    End Using
End Using

Then when you go to replace the TextBox with the ComboBox you would set the SelectedValue:

FlatType.SelectedValue = MyTextBox.Text

[jmcilhinney]

You can read the data into a data adapter, fill a data table with the data from the adapter, and then bind the data table to the combobox. This process should only happen once (unless you expect the lookup values to change), presumably during the Form's Load event.

Dim dt As New DataTable()
Using cmd As New SqlCommand("Select flattype from lkupflattype", con)
    Using adapter As New SqlDataAdapter(cmd)
        adapter.Fill(dt)
        FlatType.DataSource = dt
    End Using
End Using

Then when you go to replace the TextBox with the ComboBox you would set the SelectedValue:

FlatType.SelectedValue = MyTextBox.Text

That's still a bad idea because that's still not making use of the typed DataSet. The code should be using a table adapter, not a data adapter. You can mix and match typed DataSets and untyped DataSets if you want to but you can do all sorts of things that you shouldn't. Use a typed DataSet or don't. Don't half use it.