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

• RubberDuck: I don't know how it would be done, but I think this might be a job for linq-to-sql.
• Bjørn-Roger Kringsjå: 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.
• RubberDuck: @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.

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.

• Mathieu Guindon: 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.
• Andrew Mortimer: 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.