Results 1 to 8 of 8

Thread: Data Reader

  1. #1

    Thread Starter
    Lively Member
    Join Date
    Jul 2021
    Posts
    66

    Data Reader

    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.


    Code:
    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

  2. #2
    .NUT jmcilhinney's Avatar
    Join Date
    May 2005
    Location
    Sydney, Australia
    Posts
    107,634

    Re: Data Reader

    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:
    vb.net Code:
    1. 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:
    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:
    Code:
                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.

  3. #3

    Thread Starter
    Lively Member
    Join Date
    Jul 2021
    Posts
    66

    Re: Data Reader

    Thanks jmcilhinney - Always very helpful and to the point!

  4. #4
    Super Moderator dday9's Avatar
    Join Date
    Mar 2011
    Location
    South Louisiana
    Posts
    10,461

    Re: Data Reader

    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:
    Code:
    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
    "Code is like humor. When you have to explain it, itís bad." - Cory House
    VbLessons | Code Tags | Sword of Fury - Jameram

  5. #5
    .NUT jmcilhinney's Avatar
    Join Date
    May 2005
    Location
    Sydney, Australia
    Posts
    107,634

    Re: Data Reader

    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.

  6. #6

    Thread Starter
    Lively Member
    Join Date
    Jul 2021
    Posts
    66

    Re: Data Reader

    Thanks dday5, perfect!

    Code:
    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
    Last edited by dr225; Sep 15th, 2021 at 09:19 AM.

  7. #7
    Super Moderator dday9's Avatar
    Join Date
    Mar 2011
    Location
    South Louisiana
    Posts
    10,461

    Re: Data Reader

    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.
    Code:
    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:
    Code:
    FlatType.SelectedValue = MyTextBox.Text
    "Code is like humor. When you have to explain it, itís bad." - Cory House
    VbLessons | Code Tags | Sword of Fury - Jameram

  8. #8
    .NUT jmcilhinney's Avatar
    Join Date
    May 2005
    Location
    Sydney, Australia
    Posts
    107,634

    Re: Data Reader

    Quote Originally Posted by dday9 View Post
    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.
    Code:
    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:
    Code:
    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.

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •  



Click Here to Expand Forum to Full Width