1
\$\begingroup\$

The challenge I'm facing is creating a class for a Loan object which contains a number of properties, some of which are classes in and of themselves with similar attributes (e.g., Lender) as well as some List(Of...) properties (Borrowers and collateral). I don't want to have these classes load their properties unless specifically called for a couple of reasons:

  • Prevent unnecessary calls to the database
  • Try to prevent the use of "stale" data in the parent class when referring to these properties
  • When it's a List(Of...) property, things can get extra dicey

In order to accomplish this, I've put together the following structure, which appears to be working normally. However, I'm really beginning to wonder if I'm overthinking this and making it much more complicated than it needs to be - mostly in the List(Of...) properties. Any suggestions are most definitely welcome.

#Region "LOAN RECORD"
    ''' <summary>
    ''' Standard object containing details about a specific loan record
    ''' </summary>
    Public Class Loan
#Region "PRIVATE PROPERTIES"
        <EditorBrowsable(EditorBrowsableState.Never)> <DebuggerBrowsable(DebuggerBrowsableState.Never)>
        Private LenderID As Integer

        <EditorBrowsable(EditorBrowsableState.Never)> <DebuggerBrowsable(DebuggerBrowsableState.Never)>
        Private Property BorrowerIDs As List(Of Integer)

        <EditorBrowsable(EditorBrowsableState.Never)> <DebuggerBrowsable(DebuggerBrowsableState.Never)>
        Private Property VehicleIDs As List(Of Integer)

        <EditorBrowsable(EditorBrowsableState.Never)> <DebuggerBrowsable(DebuggerBrowsableState.Never)>
        Private Property RealEstatePropertyIDs As List(Of Integer)

#Region "BUFFERS TO PREVENT UNNECESSARY DATABASE CALLS"
        <EditorBrowsable(EditorBrowsableState.Never)> <DebuggerBrowsable(DebuggerBrowsableState.Never)>
        Private _Lender As Lender

        <EditorBrowsable(EditorBrowsableState.Never)> <DebuggerBrowsable(DebuggerBrowsableState.Never)>
        Private _Borrowers As List(Of Borrower)

        <EditorBrowsable(EditorBrowsableState.Never)> <DebuggerBrowsable(DebuggerBrowsableState.Never)>
        Private _Vehicles As List(Of VehicleCollateral)

        <EditorBrowsable(EditorBrowsableState.Never)> <DebuggerBrowsable(DebuggerBrowsableState.Never)>
        Private _RealEstateProperties As List(Of RealEstateCollateral)
#End Region
#End Region

#Region "PUBLIC PROPERTIES"
        Public Property LoanID As Integer
        Public Property LoanNumber As String
        Public Property InceptionDate As Nullable(Of Date)
        Public Property MaturityDate As Nullable(Of Date)
        Public Property CurrentBalance As Decimal
        Public Property CreditLimit As Decimal
        Public Property InterestRate As Decimal
        Public Property PaymentFrequency As Integer
        Public Property CurrentPaymentAmount As Decimal
        Public Property PaidDate As Nullable(Of Date)

        <DebuggerBrowsable(DebuggerBrowsableState.Never)>
        Public Property Lender As Lender
            Get
                If Me._Lender Is Nothing OrElse Me._Lender.LenderID <> Me.LenderID Then
                    Me._Lender = New Lender(Me.LenderID)
                End If

                Return Me._Lender
            End Get

            Set(value As Lender)
                Me.LenderID = value.LenderID
                Me._Lender = value
            End Set
        End Property

        <DebuggerBrowsable(DebuggerBrowsableState.Never)>
        Public Property Borrowers As List(Of Borrower)
            Get
                If Not Me.BorrowerIDs Is Nothing AndAlso Me.BorrowerIDs.Count > 0 Then
                    If Me._Borrowers Is Nothing OrElse Me._Borrowers.Count <> Me.BorrowerIDs.Count Then
                        Dim LoanBorrowers As New List(Of Borrower)

                        For Each BorrowerID As Integer In Me.BorrowerIDs
                            If BorrowerID > 0 Then
                                LoanBorrowers.Add(New Borrower(BorrowerID))
                            End If
                        Next BorrowerID

                        If LoanBorrowers.Count > 0 Then
                            If Not Me._Borrowers Is Nothing Then
                                Me._Borrowers.Clear()
                            Else
                                Me._Borrowers = New List(Of Borrower)
                                Me._Borrowers.Clear()
                            End If

                            Me._Borrowers = LoanBorrowers
                        Else
                            Me._Borrowers = Nothing
                        End If
                    Else
                        Dim Reload As Boolean = False

                        For Each LoanBorrower As Borrower In Me._Borrowers
                            If Not Me.BorrowerIDs.Contains(LoanBorrower.BorrowerID) Then
                                Reload = True
                                Exit For
                            End If
                        Next LoanBorrower

                        If Reload Then
                            Dim LoanBorrowers As New List(Of Borrower)

                            For Each BorrowerID As Integer In Me.BorrowerIDs
                                If BorrowerID > 0 Then
                                    LoanBorrowers.Add(New Borrower(BorrowerID))
                                End If
                            Next BorrowerID

                            If LoanBorrowers.Count > 0 Then
                                If Not Me._Borrowers Is Nothing Then
                                    Me._Borrowers.Clear()
                                Else
                                    Me._Borrowers = New List(Of Borrower)
                                    Me._Borrowers.Clear()
                                End If

                                Me._Borrowers = LoanBorrowers
                            Else
                                Me._Borrowers = Nothing
                            End If
                        End If
                    End If
                Else
                    Me._Borrowers = Nothing
                End If

                Return Me._Borrowers
            End Get

            Set(value As List(Of Borrower))
                If Not value Is Nothing AndAlso value.Count > 0 Then
                    If Me.BorrowerIDs Is Nothing Then
                        Me.BorrowerIDs = New List(Of Integer)
                    End If

                    Me.BorrowerIDs.Clear()

                    For Each valueBorrower As Borrower In value
                        Me.BorrowerIDs.Add(valueBorrower.BorrowerID)
                    Next valueBorrower

                    Me._Borrowers = value
                Else
                    Me._Borrowers = Nothing
                End If
            End Set
        End Property

        <DebuggerBrowsable(DebuggerBrowsableState.Never)>
        Public Property Vehicles As List(Of VehicleCollateral)
            Get
                If Not Me.VehicleIDs Is Nothing AndAlso Me.VehicleIDs.Count > 0 Then
                    If Me._Vehicles Is Nothing OrElse Me._Vehicles.Count <> Me.VehicleIDs.Count Then
                        Dim Collateral As New List(Of VehicleCollateral)

                        For Each CollateralID As Integer In Me.VehicleIDs
                            If CollateralID > 0 Then
                                Collateral.Add(New VehicleCollateral(CollateralID))
                            End If
                        Next CollateralID

                        If Collateral.Count > 0 Then
                            If Not Me._Vehicles Is Nothing Then
                                Me._Vehicles.Clear()
                            Else
                                Me._Vehicles = New List(Of VehicleCollateral)
                                Me._Vehicles.Clear()
                            End If

                            Me._Vehicles = Collateral
                        Else
                            Me._Vehicles = Nothing
                        End If
                    Else
                        Dim Reload As Boolean = False

                        For Each Vehicle As VehicleCollateral In Me._Vehicles
                            If Not Me.VehicleIDs.Contains(Vehicle.CollateralID) Then
                                Reload = True
                                Exit For
                            End If
                        Next Vehicle

                        If Reload Then
                            Dim Collateral As New List(Of VehicleCollateral)

                            For Each CollateralID As Integer In Me.VehicleIDs
                                If CollateralID > 0 Then
                                    Collateral.Add(New VehicleCollateral(CollateralID))
                                End If
                            Next CollateralID

                            If Collateral.Count > 0 Then
                                If Not Me._Vehicles Is Nothing Then
                                    Me._Vehicles.Clear()
                                Else
                                    Me._Vehicles = New List(Of VehicleCollateral)
                                    Me._Vehicles.Clear()
                                End If

                                Me._Vehicles = Collateral
                            Else
                                Me._Vehicles = Nothing
                            End If
                        End If
                    End If
                Else
                    Me._Vehicles = Nothing
                End If

                Return Me._Vehicles
            End Get

            Set(value As List(Of VehicleCollateral))
                If Not value Is Nothing AndAlso value.Count > 0 Then
                    If Me.VehicleIDs Is Nothing Then
                        Me.VehicleIDs = New List(Of Integer)
                    End If

                    Me.VehicleIDs.Clear()

                    For Each valueCollateral As VehicleCollateral In value
                        Me.VehicleIDs.Add(valueCollateral.CollateralID)
                    Next valueCollateral

                    Me._Vehicles = value
                Else
                    Me._Vehicles = Nothing
                End If
            End Set
        End Property

        <DebuggerBrowsable(DebuggerBrowsableState.Never)>
        Public Property RealEstateProperties As List(Of RealEstateCollateral)
            Get
                If Not Me.RealEstatePropertyIDs Is Nothing AndAlso Me.RealEstatePropertyIDs.Count > 0 Then
                    If Me._RealEstateProperties Is Nothing OrElse Me._RealEstateProperties.Count <> Me.RealEstatePropertyIDs.Count Then
                        Dim Collateral As New List(Of RealEstateCollateral)

                        For Each CollateralID As Integer In Me.RealEstatePropertyIDs
                            If CollateralID > 0 Then
                                Collateral.Add(New RealEstateCollateral(CollateralID))
                            End If
                        Next CollateralID

                        If Collateral.Count > 0 Then
                            If Not Me._RealEstateProperties Is Nothing Then
                                Me._RealEstateProperties.Clear()
                            Else
                                Me._RealEstateProperties = New List(Of RealEstateCollateral)
                                Me._RealEstateProperties.Clear()
                            End If

                            Me._RealEstateProperties = Collateral
                        Else
                            Me._RealEstateProperties = Nothing
                        End If
                    Else
                        Dim Reload As Boolean = False

                        For Each REProperty As RealEstateCollateral In Me._RealEstateProperties
                            If Not Me.RealEstatePropertyIDs.Contains(REProperty.CollateralID) Then
                                Reload = True
                                Exit For
                            End If
                        Next REProperty

                        If Reload Then
                            Dim Collateral As New List(Of RealEstateCollateral)

                            For Each CollateralID As Integer In Me.RealEstatePropertyIDs
                                If CollateralID > 0 Then
                                    Collateral.Add(New RealEstateCollateral(CollateralID))
                                End If
                            Next CollateralID

                            If Collateral.Count > 0 Then
                                If Not Me._RealEstateProperties Is Nothing Then
                                    Me._RealEstateProperties.Clear()
                                Else
                                    Me._RealEstateProperties = New List(Of RealEstateCollateral)
                                    Me._RealEstateProperties.Clear()
                                End If

                                Me._RealEstateProperties = Collateral
                            Else
                                Me._RealEstateProperties = Nothing
                            End If
                        End If
                    End If
                Else
                    Me._RealEstateProperties = Nothing
                End If

                Return Me._RealEstateProperties
            End Get

            Set(value As List(Of RealEstateCollateral))
                If Not value Is Nothing AndAlso value.Count > 0 Then
                    If Me.RealEstatePropertyIDs Is Nothing Then
                        Me.RealEstatePropertyIDs = New List(Of Integer)
                    End If

                    Me.RealEstatePropertyIDs.Clear()

                    For Each valueCollateral As RealEstateCollateral In value
                        Me.RealEstatePropertyIDs.Add(valueCollateral.CollateralID)
                    Next valueCollateral

                    Me._RealEstateProperties = value
                Else
                    Me._RealEstateProperties = Nothing
                End If
            End Set
        End Property
#End Region

#Region "PUBLIC METHODS"
#Region "CONSTRUCTORS"
        ''' <summary>
        ''' Creates a new instance of a Loan object with default values
        ''' </summary>
        Public Sub New()
            Me.Initialize()
        End Sub

        ''' <summary>
        ''' Creates a new instance of a Loan object based on the internal ID assigned to the database record
        ''' </summary>
        ''' <param name="NewLoanID">Database-assigned identification number for a specific Loan record</param>
        Public Sub New(ByVal NewLoanID As Integer)
            Me.Initialize()
            Me.GetLoanDetail(NewLoanID)
        End Sub
#End Region

        Public Shared Function FindExistingLoan(ByVal LoanNumber As String, ByVal LenderID As Integer) As LoanDetail.Loan
            Dim FoundLoan As New LoanDetail.Loan

            FoundLoan.GetLoanDetail(LoanNumber, LenderID)

            Return FoundLoan
        End Function
#End Region

#Region "PRIVATE METHODS"
        ''' <summary>
        ''' Sets all of the values to default for a new instance of a Loan object
        ''' </summary>
        Private Sub Initialize()
            Me.LoanID = 0
            Me.LenderID = 0
            Me.LoanNumber = String.Empty
            Me.InceptionDate = Nothing
            Me.MaturityDate = Nothing
            Me.CurrentBalance = 0
            Me.CreditLimit = 0
            Me.InterestRate = 0
            Me.PaymentFrequency = 12
            Me.CurrentPaymentAmount = 0
            Me.PaidDate = Nothing

            Me.BorrowerIDs = New List(Of Integer)
            Me.VehicleIDs = New List(Of Integer)
            Me.RealEstatePropertyIDs = New List(Of Integer)

            Me._Lender = Nothing
            Me._Borrowers = Nothing
            Me._Vehicles = Nothing
            Me._RealEstateProperties = Nothing
        End Sub

        ''' <summary>
        ''' Populates all of the available values for the Loan object from the database record
        ''' </summary>
        ''' <param name="DBData">DataRow containing the details for the specified Loan record from the database</param>
        Private Sub Fill(ByRef DBData As DataRow)
            With DBData
                Me.LoanID = Convert.ToInt32(.Item("LoanID"))
                Me.LenderID = Convert.ToInt32(.Item("LenderID"))
                Me.LoanNumber = Convert.ToString(.Item("LoanNumber"))

                If Not IsDBNull(.Item("InceptionDate")) Then
                    Me.InceptionDate = Convert.ToDateTime(.Item("InceptionDate"))
                End If

                If Not IsDBNull(.Item("MaturityDate")) Then
                    Me.MaturityDate = Convert.ToDateTime(.Item("MaturityDate"))
                End If

                If Not IsDBNull(.Item("CurrentBalance")) Then
                    Me.CurrentBalance = Convert.ToDecimal(.Item("CurrentBalance"))
                End If

                If Not IsDBNull(.Item("CreditLimit")) Then
                    Me.CreditLimit = Convert.ToDecimal(.Item("CreditLimit"))
                End If

                If Not IsDBNull(.Item("InterestRate")) Then
                    Me.InterestRate = Convert.ToDecimal(.Item("InterestRate"))
                End If

                If Not IsDBNull(.Item("PaymentFrequency")) Then
                    Me.PaymentFrequency = Convert.ToInt32(.Item("PaymentFrequency"))
                End If

                If Not IsDBNull(.Item("CurrentPaymentAmount")) Then
                    Me.CurrentPaymentAmount = Convert.ToDecimal(.Item("CurrentPaymentAmount"))
                End If

                If Not IsDBNull(.Item("PaidDate")) Then
                    Me.PaidDate = Convert.ToDateTime(.Item("PaidDate"))
                End If
            End With

            Me.BorrowerIDs = GetLoanBorrowerIDs()
            Me.VehicleIDs = GetLoanVehicleIDs()
            Me.RealEstatePropertyIDs = GetLoanRealEstateIDs()
        End Sub

        ''' <summary>
        ''' Gets the details of a Loan record based on the internal ID assigned in the database
        ''' </summary>
        ''' <param name="CurrentLoanID">Database-assigned identification number for a specific Loan record</param>
        Private Sub GetLoanDetail(ByVal CurrentLoanID As Integer)
            Dim MyDB As New SQLDB(DBServer)
            Dim DBData As DataTable

            MyDB.Parameters.Add("requestloanid", CurrentLoanID)
            DBData = MyDB.RunProc("""Loan"".""SelectLoan""", "LoanData")

            If DBData.Rows.Count = 1 Then
                Me.Fill(DBData.Rows(0))
            ElseIf DBData.Rows.Count > 1 Then
                Throw New Exception("More than one record was found with the specified identifier.")
            End If

            MyDB.Dispose()
            DBData.Dispose()
        End Sub

        Private Function GetLoanBorrowerIDs() As List(Of Integer)
            Dim LoanBorrowers As New List(Of Integer)

            If Me.LoanID > 0 Then
                Dim MyDB As New SQLDB(DBServer)
                Dim DBData As DataTable

                MyDB.Parameters.Add("requestloanid", Me.LoanID)
                DBData = MyDB.RunProc("""Loan"".""SelectLoanBorrower""", "LoanBorrowers")

                If DBData.Rows.Count > 0 Then
                    For Each LoanBorrower As DataRow In DBData.Rows
                        LoanBorrowers.Add(Convert.ToInt32(LoanBorrower("borrowerid")))
                    Next LoanBorrower
                End If

                MyDB.Dispose()
                DBData.Dispose()
            End If

            Return LoanBorrowers
        End Function

        ''' <summary>
        ''' Retrieves details about vehicles related to a specific loan as identified by the database-assigned identification number
        ''' </summary>
        ''' <returns></returns>
        Private Function GetLoanVehicleIDs() As List(Of Integer)
            Dim LoanVehicles As New List(Of Integer)

            If Me.LoanID > 0 Then
                Dim MyDB As New SQLDB(DBServer)
                Dim DBData As DataTable

                MyDB.Parameters.Add("requestloanid", Me.LoanID)
                DBData = MyDB.RunProc("""Loan"".""SelectLoanVehicle""", "LoanVehicles")

                If DBData.Rows.Count > 0 Then
                    For Each LoanVehicle As DataRow In DBData.Rows
                        LoanVehicles.Add(Convert.ToInt32(LoanVehicle("vehicleid")))
                    Next LoanVehicle
                End If

                MyDB.Dispose()
                DBData.Dispose()
            End If

            Return LoanVehicles
        End Function

        ''' <summary>
        ''' Retrieves details about real estate properties related to a specific loan as identified by the database-assigned identification number
        ''' </summary>
        ''' <returns></returns>
        Private Function GetLoanRealEstateIDs() As List(Of Integer)
            Dim LoanProperties As New List(Of Integer)

            If Me.LoanID > 0 Then
                Dim MyDB As New SQLDB(DBServer)
                Dim DBData As DataTable

                MyDB.Parameters.Add("requestloanid", Me.LoanID)
                DBData = MyDB.RunProc("""Loan"".""SelectLoanRealEstate""", "LoanRealEstate")

                If DBData.Rows.Count > 0 Then
                    For Each LoanProperty As DataRow In DBData.Rows
                        LoanProperties.Add(Convert.ToInt32(LoanProperty("realestateid")))
                    Next LoanProperty
                End If

                MyDB.Dispose()
                DBData.Dispose()
            End If

            Return LoanProperties
        End Function
#End Region
    End Class
#End Region

NOTE: I've used the EditorBrowsable and DebuggerBrowsable attributes to prevent some properties from popping up in Visual Studio's IntelliSense to avoid confusion when using the Loan object. I've also recently added the DebuggerBrowsableState.Never parameter to most of my class properties to prevent them from attempting to load (and, thus making one of those unnecessary database calls) when I'm debugging the program. They were having some unexpected results while I was debugging, and I honestly couldn't think of a better way to handle them than to just "hide" them unless explicitly called. As I stated above, everything currently seems to be working as intended, but I just can't help feeling like I'm "overdoing" it or, at least, could be doing it better.

Also, I've removed a number of methods (the Insert, Update and Delete methods, for example) from the code below as they aren't really relevant to the specific nature of this question. Thank you for your time.

FULL DISCLOSURE: I am an entirely self-taught programmer. This code actually arose because of a few of the comments I received on a question I asked on the StackOverflow site, "VB.NET How To Prevent Infinite Recursion During Object Population".

\$\endgroup\$
  • \$\begingroup\$ <TANGENTIALLY RELATED> Well, I just stumbled across something I've not seen before that may help to eliminate the "stale" data concern. I'm using a PostgreSQL database for my backend (although, it looks like something that's available in most major RDBMS's) with the Npgsql library (v. 2.0.50727), and I just found the NOTIFY functionality. I'm wondering if this might be something to pursue to help with my overall design. \$\endgroup\$ – G_Hosa_Phat Jan 30 '18 at 15:12
1
\$\begingroup\$

General

  • Why don't you use Lazy<T> which seems to be perfect for your usage szenario.

  • Regions, well regions are usually a sign that a class may be too big. If you need regions to structure your code then you have to much code in that class and you should consider to split the class into 2 or more classes.

    At least the description of a region shouldn't lie about its content. Private LenderID As Integer is in the region named PRIVATE PROPERTIES but is clearly a variable.

  • Although VB.NET is case-insensitive you should consider to apply the .NET Naming Guidline because readers of your code will usually expect this and it will make it easier for them to read and understand your code.

    E.g methods parameter are named using camelCase casing, classlevel variables are either named using camelCase casing or are underscore prefixed. If you use underscore prefixed variablenames you shouldn't use them with the Me specifier together. Basically you should use Me only if you need to explicitly distinguish between a class level variable and e.g a method parameter which has the same name.

  • You should extract magic strings/numbers into meaningful named constants. Assume you change the name of the requestloanid parameter, it would be much easier to change this only at one place in your code. Right now you would need to change it at least at 4 places in your code.


Property Lender

If one sets the value of this property to Nothing a NullReferenceException will be thrown. Althought that might be acceptable it would be better to throw an ArgumentNullException. IMO parameters of public methods/properties should always be checked at least for Nothing.

Properties Borrowers, Vehicles, RealEstateProperties,

You have a lot of duplicated code in the getters of these properties which should be extracted to separate methods. In addition you don't need to Clear() a New List<T>.

You could think about wether you really want to set a property to Nothing, at least for the getter of the List<T> properties. If there aren't any items in the List<T> why don't you leave them as they are? Doing so would result in not having to check for Nothing from the caller side of the property.

Calling a constructor from an overloaded constructor is more .NET like and the intend of the call is more clear.

Like so

Public Sub New()
    Me.Initialize()
End Sub

Public Sub New(ByVal NewLoanID As Integer)
    Me.New()
    Me.GetLoanDetail(NewLoanID)
End Sub

But reading Me.GetLoanDetail(NewLoanID) made me wonder what a GetXX() method would do if the returned isn't assigned to anything. Thats a clear sign that you should work on your naming. IMO overloading the Fill() method would be a better way. Don't make maintainers of your code wonder what it is doing. Assume you have to add a feature or fix a bug in 3 or 6 months, you would grasp it at first glance what the call is about if you would read Me.Fill(NewLoanID).

GetLoanDetail(), GetLoanBorrowerIDs(), GetLoanVehicleIDs()

Instead of manually disposing the connection and the datatable(which you wouldn't need to dispose btw) you should use a Using statement.

GetLoanBorrowerIDs(), GetLoanVehicleIDs()

By reverting the if condition If Me.LoanID > 0 Then you could return early and would save one level of indentation. Assuming LoanID won't be negative this would look like so

Private Function GetLoanVehicleIDs() As List(Of Integer)
    Dim LoanVehicles As New List(Of Integer)

    If Me.LoanID = 0 Then
        Return LoanVehicles
    End If

    Dim MyDB As New SQLDB(DBServer)
    Dim DBData As DataTable

    MyDB.Parameters.Add("requestloanid", Me.LoanID)
    DBData = MyDB.RunProc("""Loan"".""SelectLoanVehicle""", "LoanVehicles")

    If DBData.Rows.Count > 0 Then
        For Each LoanVehicle As DataRow In DBData.Rows
            LoanVehicles.Add(Convert.ToInt32(LoanVehicle("vehicleid")))
        Next LoanVehicle
    End If

    MyDB.Dispose()
    DBData.Dispose()

    Return LoanVehicles
End Function
\$\endgroup\$
  • \$\begingroup\$ Thank you for the detailed review. You've provided me a lot to ponder. I'll have to look at Lazy<T> as it isn't anything I've yet encountered. \$\endgroup\$ – G_Hosa_Phat Feb 5 '18 at 7:21
  • \$\begingroup\$ @Heslacher - I have a quick question related to your suggestion about the usage of Lazy<T>: Is your suggestion to change my Property declarations? For example, change Public Property Lender As Lender to Public Property Lender As Lazy(Of Lender) and then remove some of the "extra" private properties? \$\endgroup\$ – G_Hosa_Phat Feb 7 '18 at 21:00

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.