5
\$\begingroup\$

My project creates a report based on test results which are stored in database table associated with a JobNumber. The test results for a particular JobNumber may be all be from the same date or from a range of dates. I create a report to display results for a single JobNumber and this report contains a string to display this date or range in a readable way.

The following code would produce the date range string from the two classes outlined below and the two interfaces implied below:

With ReportRepository.Create("1234")
    Debug.print .MonitoringDates
End With

The desired output of IReportRepository.MonitoringDates:

  • "01 January 2022" - single date
  • "01 - 02 January 2022" - date range in same month
  • "01 January - 01 February 2022" - date range across different months
  • "01 January 2022 - 01 January 2023" - date range across different years

The project uses Rubberduck SecureADODB - https://github.com/rubberduck-vba/examples/tree/master/SecureADODB

SecureDatabase class, Implements IDataAccessObject:

'@Folder "ProjectWriter.DataAccessObject"
Option Explicit

Implements IDataAccessObject

Private Type TSecureDatabase
    SecureConnetion As IDbConnection
    Command As IDbCommand
End Type

Private this As TSecureDatabase

Const ConnectionString As String = "<connection string here>"

Private Sub Class_Initialize()
    Dim Mappings As ITypeMap
    Set Mappings = AdoTypeMappings.Default
    
    Dim Provider As IParameterProvider
    Set Provider = AdoParameterProvider.Create(Mappings)
    
    Dim BaseCommand As IDbCommandBase
    Set BaseCommand = DbCommandBase.Create(Provider)

    With DbConnection.Create(ConnectionString)
        Set this.Command = DefaultDbCommand.Create(.Self, BaseCommand)
    End With
End Sub

Private Function IDataAccessObject_MonitoringStartDate(ByVal JobNumber As String) As Date
    AllErrors.GuardEmptyString JobNumber
    
    IDataAccessObject_MonitoringStartDate = this.Command.GetSingleValue("SELECT MIN(SampleDate) FROM Results JOIN Jobs ON Results.JobID = Jobs.ID WHERE Jobs.JobNumber = ? LIMIT 1", JobNumber)
End Function

Private Function IDataAccessObject_MonitoringEndDate(ByVal JobNumber As String) As Date
    AllErrors.GuardEmptyString JobNumber
    
    IDataAccessObject_MonitoringEndDate = this.Command.GetSingleValue("SELECT MAX(SampleDate) FROM Results JOIN Jobs ON Results.JobID = Jobs.ID WHERE Jobs.JobNumber = ? LIMIT 1", JobNumber)
End Function

'other functions

ReportRepository class, Implements IReportRepository

'@Folder("ProjectWriter.Repository")
'@PredeclaredId
Option Explicit

Implements IReportRepository

Private Type TReportRepository
    DataAccessObject As IDataAccessObject
    MonitoringDates As String
    'other members
End Type

Private this As TReportRepository

Public Property Get Self() As ReportRepository
    Set Self = Me
End Property

Public Function Create(ByVal JobNumber As String) As IReportRepository
    AllErrors.GuardEmptyString JobNumber
    
    With New ReportRepository
        .Initialise JobNumber
        Set Create = .Self
    End With
End Function

Public Sub Initialise(ByVal JobNumber As String)
    this.JobNumber = JobNumber
    Set this.DataAccessObject = New SecureDatabase
    With this.DataAccessObject
        this.MonitoringDates = MonitoringDates(.MonitoringStartDate(JobNumber), .MonitoringEndDate(JobNumber))
        'initialisation of other members
    End With
End Sub

Private Function MonitoringDates(ByVal StartDate As Date, ByVal EndDate As Date) As String
    Dim StartMonth As String
    StartMonth = VBA.Format$(StartDate, "mmmm")
    Dim EndMonth As String
    EndMonth = VBA.Format$(EndDate, "mmmm")
    
    Dim StartYear As String
    StartYear = VBA.Format$(StartDate, "yyyy")
    Dim EndYear As String
    EndYear = VBA.Format$(EndDate, "yyyy")
    
    Dim Dates As String
    Select Case True
        Case EndDate = StartDate:
            Dates = VBA.Format$(StartDate, "dd mmmm yyyy")
        Case StartMonth = EndMonth And StartYear = EndYear
            Dates = VBA.Format$(StartDate, "dd") & " - " & VBA.Format$(EndDate, "dd mmmm yyyy")
        Case StartYear = EndYear
            Dates = VBA.Format$(StartDate, "dd mmmm") & " - " & VBA.Format$(EndDate, "dd mmmm yyyy")
        Case Else:
            Dates = VBA.Format$(StartDate, "dd mmmm yyyy") & " - " & VBA.Format$(EndDate, "dd mmmm yyyy")
    End Select
    
    MonitoringDates = Dates
End Function

Private Property Get IReportRepository_MonitoringDates() As String
    IReportRepository_MonitoringDates = this.MonitoringDates
End Property

'other properties

I invite your comments, particularly on:

  1. If I am using the SecureADODB correctly - I was thinking there may be a way to refactor what I am doing to include both the start and end date in a single transaction or query and whether this would be worthwhile.
  2. If there is a more efficient way of producing the date range string compared to the VBA select case.
  3. Any comments on the structure of the classes for what I appear to be doing. Or any other comments on how I can improve the way this code is written.

Sorry if I have missed anything that I should have included, this is my first code review post. Thanks in advance!

\$\endgroup\$

1 Answer 1

5
\$\begingroup\$

Regarding the points of interest you identified:

  1. Using either one or two queries to get your data can be acceptable so long as it meets your performance criteria in actual use. However, I am going to suggest that you package the two results (Start and End dates) within a data object so that the rest of your code can operate on a single context of data from the database.

  2. More efficient way to produce the date range string: Using Select Case can become a problem if the number of possible cases can continue to grow over time. It would seem there is only a finite number of ways that the range string will be represented...So, using Select Case looks OK here. On the other hand, the order of the Case statements matter here because they do not specify entirely independent cases. Because of this, I would use If-ElseIf-Else instead because it makes this overlapping condition more obvious. Also, there are some opportunities to apply the Don't Repeat Yourself (DRY) principle within the date range string code.

  3. Structure: There are some structural changes that I am suggesting for you to consider. Instantiating a SecureDatabase instance inside the ReportRepository object would be considered inconsistent with SOLID coding practices (in particular, the letters "L" and "D"). In the code below, the ReportRepository is rewritten to consume an IDataAccessObject reference and a JobNumber in its Create function input parameters.

  4. (Testing) A consequence of creating the SecureDatabase instance within the ReportRepository object is that it is impossible to test the code without the existence of a database. And, accordingly, I noticed a Sample database identifier within the SQL statements of the provided code. Passing in an IDataAccessObject facilitates testing free of a database (Sample or otherwise) and is more consistent with SOLID coding practices. Further, in the spirit of easy testing, the MonitoringDates function was made Public to allow testing without having to call ReportRepository.Create or have a database.

The code below is a revised version incorporating the points described above.

ReportRepository

'@PredeclaredId
'@Folder("ProjectWriter.Repository")
Option Explicit

Implements IReportRepository

Private Type TReportRepository
    MonitoringDates As String
    JobNumber As String
    StartDate As Date
    EndDate As Date
    SecureDatabase As IDataAccessObject
End Type

Private this As TReportRepository

Public Property Get Self() As ReportRepository
    Set Self = Me
End Property

Public Function Create(ByVal dDB As IDataAccessObject, ByVal JobNumber As String) As IReportRepository
    With New ReportRepository
        .Initialise dDB, JobNumber
        Set Create = .Self
    End With
End Function

Public Sub Initialise(ByVal dDB As IDataAccessObject, ByVal JobNumber As String)
    Set this.SecureDatabase = dDB
    this.JobNumber = JobNumber
    
    Dim dataObject As DateRangeDTO
    Set dataObject = this.SecureDatabase.RetrieveDateRangeDTO(JobNumber)
    
    this.StartDate = dataObject.StartDate
    this.EndDate = dataObject.EndDate
    this.MonitoringDates = MonitoringDates(dataObject.StartDate, dataObject.EndDate)
End Sub

Public Function MonitoringDates(ByVal vbaStartDate As Date, ByVal vbaEndDate As Date) As String

    Dim dStartDateFormat As String
    dStartDateFormat = "dd mmmm yyyy"
    
    Dim endDateExpression As String
    endDateExpression = VBA.Format$(vbaEndDate, dStartDateFormat)

    Dim toEndDateExpression As String
    toEndDateExpression = " - " & endDateExpression
    
    If vbaStartDate = vbaEndDate Then
        MonitoringDates = endDateExpression
    ElseIf VBA.Year(vbaStartDate) = VBA.Year(vbaEndDate) Then
        
        dStartDateFormat = IIf(VBA.Month(vbaStartDate) = VBA.Month(vbaEndDate), "dd", "dd mmmm")
        
        MonitoringDates = VBA.Format$(vbaStartDate, dStartDateFormat) & toEndDateExpression
    Else
        MonitoringDates = VBA.Format$(vbaStartDate, dStartDateFormat) & toEndDateExpression
    End If

End Function

Private Property Get IReportRepository_MonitoringDates() As String
    IReportRepository_MonitoringDates = this.MonitoringDates
End Property

'other properties

IDataAccessObject IDataAccessObject was modified to return a simple data object

Public Function RetrieveDateRangeDTO(ByVal JobNumber As String) As DateRangeDTO
End Function

DateRangeDTO DateRangeDTO keeps the Job number, start and end dates as a single context.

Option Explicit

Private Type TDateRangeDTO
    JobNumber As String
    StartDate As Date
    EndDate As Date
End Type

Private this As TDateRangeDTO

Public Property Get JobNumber() As String
    JobNumber = this.JobNumber
End Property
Public Property Let JobNumber(ByVal RHS As String)
    this.JobNumber = RHS
End Property

Public Property Get StartDate() As Date
    StartDate = this.StartDate
End Property
Public Property Let StartDate(ByVal RHS As Date)
    this.StartDate = RHS
End Property

Public Property Get EndDate() As Date
    EndDate = this.EndDate
End Property
Public Property Let EndDate(ByVal RHS As Date)
    this.EndDate = RHS
End Property

An example of testing without a database using a Fake object

SecureDatabaseFake


Implements IDataAccessObject

Private Type TSecureDatabaseFake
    TestDTO As DateRangeDTO
End Type

Private this As TSecureDatabaseFake

Const ConnectionString As String = "<connection string here>"

Private Function IDataAccessObject_RetrieveDateRangeDTO(ByVal testJobNumber As String) As DateRangeDTO
    this.TestDTO.JobNumber = testJobNumber
    Set IDataAccessObject_RetrieveDateRangeDTOO = this.TestDTO
End Function

Public Sub LoadDateRangeDTO(ByVal testDataDTO As DateRangeDTO)
    Set this.TestDTO = testDataDTO
End Sub

A couple tests made possible by using the database Fake and other changes:

Option Explicit


Sub Test()

    'Arrange
    Dim dDBFake As SecureDatabaseFake
    Set dDBFake = New SecureDatabaseFake

    Dim dDB As IDataAccessObject
    Set dDB = dDBFake

    Dim dDTO As DateRangeDTO
    Set dDTO = New DateRangeDTO
    dDTO.StartDate = DateValue("January 3, 2022")
    dDTO.EndDate = DateValue("August 4, 2045")

    'Specify the results that the fake database should return
    dDBFake.LoadDateRangeDTO dDTO

    'Act
    Dim dRep As IReportRepository
    Set dRep = ReportRepository.Create(dDB, "1234")
    
    'Assert
    If dRep.MonitoringDates <> "03 January 2022 - 04 August 2045" Then
        MsgBox "Test Failed"
        Exit Sub
    End If

    MsgBox "TestPassed"

End Sub

Sub TestMonitoringDatesDirectly()
    Dim dTestResult As String
    
    dTestResult = ReportRepository.MonitoringDates(DateValue("January 3, 2022"), DateValue("January 14, 2022"))
    If dTestResult <> "03 - 14 January 2022" Then
        MsgBox "Test Failed"
        Exit Sub
    End If

    MsgBox "TestPassed"
End Sub
\$\endgroup\$
1
  • \$\begingroup\$ Thanks very much for taking the time to answer and putting in the examples - it makes a lot of sense. I have now implemented your suggestions 2-3 in my project. I'm still looking to implement the DTO, just thinking over the best way to do it with respect to the rest of the classes which I haven't included above. \$\endgroup\$
    – Energeer
    Jan 16 at 13:57

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.