Get list from database in 3 layer model

Question

I'm working on some testing project. For some reason I think I'm doing something wrong with the architecture. I have a simple example to show what I mean.

Database class

Imports DAO
Imports System.Data.SqlClient

Public Class PatientDB

    Dim connectionString As String = "Data Source=GSABBOZMAINPC;Initial Catalog=TestDB;Integrated Security=True"

    Public Function GetAllPatientFromDB() As List(Of PatientDAO)

        Using connection As New SqlConnection(connectionString)


            Dim list As New List(Of PatientDAO)

            Dim sqlString As String = "select * from patient ;"

            Using Command As New SqlCommand(sqlString, connection)

                connection.Open()

                Using reader As SqlDataReader = Command.ExecuteReader

                    While reader.Read

                        Dim firstName As String = reader("firstnamePT").ToString()

                        Dim Name As String = reader("naamPT").ToString()

                        Dim patient As New PatientDAO(Name, firstName)

                        list.Add(patient)

                    End While

                End Using

                Return list

            End Using

        End Using  

    End Function

End Class

PatientDAO

Public Class PatientDAO

    Private _name As String
    Private _lastname As String


    Sub New(naam As String, lastname As String)

        _name = naam
        _lastname = lastname

    End Sub

    Public ReadOnly Property Name As String
        Get
            Return _name
        End Get
    End Property

    Public ReadOnly Property Lastname As String
        Get

            Return _lastname

        End Get
    End Property

End Class

Patient

Imports DAO
Imports database_layer

    Public Class Patient

        Property naam As String

        Property lastname As String


        Sub New()

        End Sub

        Sub New(_naam As String, _lastname As String)

            naam = _naam

            lastname = _lastname

        End Sub

        Public Sub New(patientDAO As PatientDAO)

            naam = patientDAO.Name
            lastname = patientDAO.Lastname

        End Sub

        Public Function GetDO() As PatientDAO

            Return New PatientDAO(naam, lastname)


        End Function


        Public Function getAllPatientsInList() As List(Of Patient)

            Dim list As New List(Of Patient)
            Dim database As New PatientDB


            For Each PatientDAO As PatientDAO In database.GetAllPatientFromDB()

                Dim patient As New Patient(PatientDAO)
                list.Add(patient)
            Next

            Return list


        End Function

        Public Overrides Function ToString() As String
            Return naam + " " + lastname
        End Function

    End Class

In this example I'm using Windows form. When the form is loaded I use the following event:

Private Sub Form1_Load(sender As Object, e As EventArgs) Handles Me.Load

    Dim patient As New Patient

    For Each item As Patient In patient.getAllPatientsInList()

        list.Add(item)

    Next

End Sub

Should I use Dim patient As New Patient with an empty constructor like this example or should I approach this another way? And are there other suggestions you think would improve the architecture of this example?

Answer 1

Well, you were right to think you are wrong. We shouldn't need to instantiate an object with an empty constructor to something like this. This is exactly what the Shared keyword is for.

First, make your getAllPatientsInList function Shared:

Public Shared Function GetAllPatients() As List(Of Patient)
    ' method implementation
End Function

And now we can call the method without instantiating an instance of the class:

For Each item as Patient in Patient.GetAllPatients()
    ' do something with item
Next 

And once we've done this, arguably, we may want to eliminate the default, zero-argument constructor.

Answer 2

I would highly recommend you take the method that returns a list of patients off of the patient class. An entity-type object is not responsible for fetching multiple instances of its type from a datasource, generally. Plus, your current implementation makes the patient (the layer closest to the UI) have a dependency on the data source, which is the farthest you can get from the UI.

At a high level, I would recommend structuring your flow this way:

• Create an object in the middle layer (service, repo, whatever you want to call it) that encapsulates the DB call. It fetches a list of patient DTOs from the database and manages it internally. This object will at the very minimum map the list of patient DTOs to a list of Patient objects but there may be some additional processing you might want to do on the patients you get from the DB. In that case, the logic probably belongs in this object.

• If this object gets too bloated, break out different functionality like mapping, etc. into other classes.

• Return the mapped list to the UI

• Bind to your nice simple Patient list on the client