Populating fields from stored procedure

Question

I was just wondering if my code below is a good way of populating data from database. I have had no issues with this really but I am being taught this and just wondering if it is the best practice.

 Public Sub LoadProperty()
    '**Finds the Current Property Data

    'Error Checking
    ' On Error GoTo Err_LoadProperty
    Try

        Dim uRecSnap As ADODB.Recordset


        ' Check For Open Connection
        If uDBase Is Nothing Then
            OpenConnection()
            bConnection = True
        End If

        ' Run Stored Procedure - Load Property Record
        uCommand = New ADODB.Command
        With uCommand
            .ActiveConnection = uDBase
            .CommandType = ADODB.CommandTypeEnum.adCmdStoredProc
            .CommandTimeout = 0
                 uCommand.Parameters.Append(uCommand.CreateParameter("@PropertyID", ADODB.DataTypeEnum.adInteger, ADODB.ParameterDirectionEnum.adParamInput, , Val(lblPropertyIDValue.Text)))
            .CommandText = "PropertyMaster_LoadRecord"

            uRecSnap = .Execute
        End With

        ' Store Data Values
        Do Until uRecSnap.EOF
            lblPropertyIDValue.Text = If(IsDBNull(uRecSnap("PropertyID").Value), "", uRecSnap("PropertyID").Value)
            lblSPMReferenceValue.Text = If(IsDBNull(uRecSnap("SPMReference").Value), "", uRecSnap("SPMReference").Value)
            cmbPropertyManager.Text = If(IsDBNull(uRecSnap("PropertyManager").Value), "", uRecSnap("PropertyManager").Value)
            txtAddress1.Text = If(IsDBNull(uRecSnap("AddressLine1").Value), "", uRecSnap("AddressLine1").Value)
            txtAddress2.Text = If(IsDBNull(uRecSnap("AddressLine2").Value), "", uRecSnap("AddressLine2").Value)
            txtAddress3.Text = If(IsDBNull(uRecSnap("AddressLine3").Value), "", uRecSnap("AddressLine3").Value)
            txtTown.Text = If(IsDBNull(uRecSnap("Town").Value), "", uRecSnap("Town").Value)
            txtPostCode.Text = If(IsDBNull(uRecSnap("PostCode").Value), "", uRecSnap("PostCode").Value)
            chkAvailable.Checked = If(IsDBNull(uRecSnap("Availabilty").Value), "", uRecSnap("Availabilty").Value)
            dtpAvailable.Value = If(IsDBNull(uRecSnap("Available").Value), "1/1/1900", uRecSnap("Available").Value)
            chkFactored.Checked = If(IsDBNull(uRecSnap("Factored").Value), 0, uRecSnap("Factored").Value)
            txtFactorName.Text = If(IsDBNull(uRecSnap("FactorsName").Value), "", uRecSnap("FactorsName").Value)
            txtFactorsEmail.Text = If(IsDBNull(uRecSnap("FactorsEmail").Value), "", uRecSnap("FactorsEmail").Value)
            dtpPropertyBuilt.Value = If(IsDBNull(uRecSnap("PropertyBuilt").Value), "1/1/1900", uRecSnap("PropertyBuilt").Value)
            txtPropertyValue.Text = If(IsDBNull(uRecSnap("PropertyValue").Value), "", uRecSnap("PropertyValue").Value)
            txtMinimumFee.Text = If(IsDBNull(uRecSnap("MimimumFee").Value), "", uRecSnap("MimimumFee").Value)
            txtCostAuthorisationAmount.Text = If(IsDBNull(uRecSnap("CostSuthorisationAmount").Value), "", uRecSnap("CostSuthorisationAmount").Value)
            txtCommision.Text = If(IsDBNull(uRecSnap("Commission").Value), "", uRecSnap("Commission").Value)
            chkVacant.Checked = If(IsDBNull(uRecSnap("Vacant").Value), 0, uRecSnap("Vacant").Value)
            dtpVacant.Value = If(IsDBNull(uRecSnap("VacantDate").Value), "1/1/1900", uRecSnap("VacantDate").Value)
            txtStartingRent.Text = If(IsDBNull(uRecSnap("StartingRent").Value), "", uRecSnap("StartingRent").Value)

            uRecSnap.MoveNext()
        Loop

        ' Close Connection
        uRecSnap.Close()
        If bConnection Then CloseConnection()
        uRecSnap = Nothing


    Catch ex As Exception
        MsgBox(vbCrLf & ex.Message & vbCrLf & "Source---" & vbCrLf & ex.Source & vbCrLf & "Problem---" & vbCrLf & ex.StackTrace, MsgBoxStyle.Exclamation + MsgBoxStyle.OkOnly, "SouthSide- Error Reporting")

    End Try



End Sub

Answer 1

It might have been an ok-ish way of doing things... 15 years ago. This code is astonishingly similar to what one would be writing in vba - minus the Try..End Try block.

If you want to learn the .NET way of doing things, I strongly recommend you remove this from every VB.NET module you're writing:

Imports Microsoft.VisualBasic

Things in that namespace exist to make an easier transition between vb6/vba and vb.net - pretty much everything in it has an actual .net-idiomatic equivalent elsewhere in the framework.

• ADODB / ActiveX Data Objects is replaced with ado.net, itself rendered obsolete with linq-to-sql, itself a predecessor of entity-framework. But one step at a time - start with ado.net.
• MsgBox is replaced with System.Windows.Forms.MessageBox
• vbCrLf should have been vbNewLine, and both are replaced with Environment.NewLine

---

You're not including the module-scope fields, and yet the code you posted seems to use plenty... but I don't understand why. You're opening a module-scoped connection, fetching data, and then closing it. Why would that object ever need to exist beyond that? You don't need that Boolean bConnection.

ADODB being COM / unmanaged, it doens't implement the IDisposable interface. See this Stack Overflow answer for information about proper cleanup of COM objects... which should motivate you to use managed code ;-)

In short:

Use Marshal.ReleaseComObject to tidy up COM objects. ADODB is a COM library being used through COM Interop.

---

Now, what you have here is essentially a Smart UI pattern: the UI runs the show, and controls everything. That's good for learning how things work, fiddling around and prototyping things, but it's also one of the best ways to create a spaghetti mess out of production code.

Your form should only be responsible for one thing: being a user interface. Accessing a database is not the job of a UI, it's that of some data service.

You should create a class to act as your Model, that would expose a property for each value you're fetching from a database record.

Then the View (/form) wouldn't need to know about a database or connections or anything - it would simply display what the Model has in store for it. And then when the user makes changes, you can take the modified Model and pass it to the same data service that provided you with it, to write the modified values into the database.

Read up on the Model-View-Presenter pattern for more details.

Answer 2

Consider adding a finally block to safely close the connection.