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