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

        Dim uRecSnap As ADODB.Recordset

        ' Check For Open Connection
        If uDBase Is Nothing Then
            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)


        ' Close Connection
        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
  • \$\begingroup\$ I don't know how it would be done, but I think this might be a job for linq-to-sql. \$\endgroup\$ – RubberDuck Feb 6 '15 at 13:34
  • 1
    \$\begingroup\$ Are you sure this is vb.net? ADODB, Recordset, EOF, MsgBox, vbCrLf? I would expect to see <Db>/Connection/Command/Reader, MessageBox and Environment.NewLine. \$\endgroup\$ – Bjørn-Roger Kringsjå Feb 7 '15 at 11:56
  • 2
    \$\begingroup\$ @Bjørn-RogerKringsjå it's vb.net. I've had prior experience with this OP posting vb.net code under the VB6 tag. I would say those are all valid points of review. \$\endgroup\$ – RubberDuck Feb 18 '15 at 20:31

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 - 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 / and - pretty much everything in it has an actual -idiomatic equivalent elsewhere in the framework.

  • ADODB / ActiveX Data Objects is replaced with , itself rendered obsolete with , itself a predecessor of . But one step at a time - start with .
  • 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.

|improve this answer|||||

Consider adding a finally block to safely close the connection.

|improve this answer|||||
  • 4
    \$\begingroup\$ That would be a very valid point with an ADO.NET connection, although a Using block would be arguably cleaner - thing is, it's an ADODB connection... which isn't IDisposable. And it's module-scoped, which suggests it's being used in other methods of the same class; disposing it would render it useless. \$\endgroup\$ – Mathieu Guindon May 30 '15 at 4:00
  • 2
    \$\begingroup\$ The finally block is to ensure the connection is managed, not to trigger the non-existent dispose method. I've removed the misleading word 'dispose' to prevent further confusion. \$\endgroup\$ – Andrew Mortimer May 31 '15 at 7:10

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.