6
\$\begingroup\$

Some fraction of a follow-up to The half-finished version.

What's changed: Added year as well as Day/Month. Added input Validation. Implemented a poor man's .EnableEvents = false for UserForms. Re-jigged the event heirarchy (Change year --> Repopulate Months or Days, Change Months --> Repopulate Days).

As always, all feedback welcomed.

In particular, if you were given this code to maintain, what would you be thinking as you read through it?

Initialisation and populating control values:

Option Explicit

Private Userform_EnableEvents As Boolean

Private Sub UserForm_Initialize()

    Userform_EnableEvents = True

    PopulateYearBox Me.UF_BankRec_cbx_Year

End Sub

Private Sub PopulateYearBox(ByRef yearBox As MSForms.ComboBox)
    DisableFormEvents

        Dim ixYear As Long

            For ixYear = 2000 To Year(Now)
                yearBox.AddItem ixYear
            Next ixYear

    EnableFormEvents
End Sub

Private Sub PopulateMonthBox(ByRef monthBox As MSForms.ComboBox, ByVal yearText As String)
    DisableFormEvents

    Dim ixYear As Long
        ixYear = CLng(yearText)

    Dim monthText As String
        monthText = monthBox.Text

    Dim ixMonth As Long, ixFinalMonth As Long
        If ixYear = Year(Now) Then
            ixFinalMonth = Month(Now)
        Else
            ixFinalMonth = 12
        End If

        monthBox.Clear
        For ixMonth = 1 To ixFinalMonth
            monthText = MonthName(ixMonth)
            monthBox.AddItem monthText
        Next ixMonth

    EnableFormEvents
End Sub

Private Sub PopulateDayBox(ByRef dayBox As MSForms.ComboBox, ByVal monthText As String, ByVal yearText As String)
    DisableFormEvents

    Dim dateCounter As Date, startDate As Date
        startDate = CDate("01/" & monthText & "/" & yearText)

        dateCounter = startDate
        dayBox.Clear
        dayBox.AddItem Day(dateCounter)
        dateCounter = dateCounter + 1
        Do While Month(dateCounter) = Month(dateCounter - 1)
            dayBox.AddItem Day(dateCounter)
            dateCounter = dateCounter + 1
        Loop

    EnableFormEvents
End Sub

Value_Change event triggers

Private Sub UF_BankRec_cbx_Year_Change()
    If Userform_EnableEvents Then

        DisableFormEvents

        Dim dayBox As MSForms.ComboBox
        Set dayBox = Me.UF_BankRec_cbx_EndDay

        Dim monthBox As MSForms.ComboBox
        Set monthBox = Me.UF_BankRec_cbx_Month

        Dim monthText As String
            monthText = monthBox.Text

        Dim yearText As String, ixYear As Long
            yearText = Me.UF_BankRec_cbx_Year.Text
            ixYear = CLng(yearText)

            If monthBox.ListCount <> 12 Or ixYear = Year(Now) Then
                PopulateMonthBox monthBox, yearText
            Else
                PopulateDayBox dayBox, monthText, yearText
            End If

        EnableFormEvents

    End If
End Sub

Private Sub UF_BankRec_cbx_Month_Change()
    If Userform_EnableEvents Then
        DisableFormEvents

        Dim dayBox As MSForms.ComboBox
        Set dayBox = Me.UF_BankRec_cbx_EndDay

        Dim monthBox As MSForms.ComboBox
        Set monthBox = Me.UF_BankRec_cbx_Month

        Dim yearBox As MSForms.ComboBox
        Set yearBox = Me.UF_BankRec_cbx_Year

        Dim monthText As String
            monthText = monthBox.Text

        Dim yearText As String
            yearText = yearBox.Text


            If yearBox.Text <> "" Then
                dayBox.Clear
                PopulateDayBox dayBox, monthText, yearText
            End If

        EnableFormEvents
    End If
End Sub

Private Sub DisableFormEvents()

    Userform_EnableEvents = False

End Sub

Private Sub EnableFormEvents()

    Userform_EnableEvents = True

End Sub

Exit Point

Private Sub UF_BankRec_btn_RetrieveData_Click()

    Dim yearBox As MSForms.ComboBox, monthBox As MSForms.ComboBox, dayBox As MSForms.ComboBox, cellSelectionBox As RefEdit.RefEdit
    Dim yearText As String, monthText As String, dayText As String
    Dim ixYear As Long, ixMonth As Long, ixDay As Long
    Dim startDate As Date, endDate As Long

        Set yearBox = Me.UF_BankRec_cbx_Year
        Set monthBox = Me.UF_BankRec_cbx_Month
        Set dayBox = Me.UF_BankRec_cbx_EndDay
        Set cellSelectionBox = Me.UF_BankRec_ref_TitleCell

        ValidateControlInputs dayBox, monthBox, yearBox, cellSelectionBox

        yearText = yearBox.Text
        monthText = monthBox.Text
        dayText = dayBox.Text

        ixYear = Year("01/01/" & yearText)
        ixMonth = Month("01/" & monthText & "/2000")
        ixDay = CLng(dayText)

        startDate = DateSerial(ixYear, ixMonth, 1)
        endDate = DateSerial(ixYear, ixMonth, ixDay)

    Dim cellAddress As String, rngTitleCell As Range

        cellAddress = cellSelectionBox.value
        Set rngTitleCell = Range(cellAddress)

        GetBankRecData 'rngTitleCell, startDate, endDate

End Sub

Data Validation

Private Sub ValidateControlInputs(ByRef dayBox As MSForms.ComboBox, ByRef monthBox As MSForms.ComboBox, ByRef yearBox As MSForms.ComboBox, ByRef cellSelectionBox As RefEdit.RefEdit)

    ValidateDayBox dayBox

    ValidateMonthBox monthBox

    ValidateYearBox yearBox

    ValidateCellSelectionBox cellSelectionBox

End Sub

Private Sub ValidateDayBox(ByRef dayBox As MSForms.ComboBox)

    Dim dayString As String
        dayString = dayBox.Text

    Dim passedValidation As Boolean
        passedValidation = False

    Dim finalDay As Long
        finalDay = (dayBox.ListCount - 1)

    Dim strErrorMessage As String

        passedValidation = dayString <= finalDay And (dayString Like "#" Or dayString Like "##")

        If Not passedValidation Then
            strErrorMessage = "The selected day is invalid. Please select a valid date."
            PrintErrorMessage strErrorMessage
        End If

End Sub

Private Sub ValidateMonthBox(ByRef monthBox As MSForms.ComboBox)

    Dim monthString As String
        monthString = monthBox.Text

    Dim passedValidation As Boolean
        passedValidation = False

    Dim strErrorMessage As String

    Dim i As Long, strMonth As String

        passedValidation = False
        For i = 1 To 12
            strMonth = MonthName(i)
            If strMonth = monthString Then passedValidation = True
        Next i

        If Not passedValidation Then
            strErrorMessage = "Please Select a valid month"
            PrintErrorMessage strErrorMessage
        End If

End Sub

Private Sub ValidateYearBox(ByRef yearBox As MSForms.ComboBox)

    Dim yearString As String
        yearString = yearBox.Text

    Dim passedValidation As Boolean
        passedValidation = False

    Dim lngYear As Long, currentYear As Long
        lngYear = CLng(yearString)
        currentYear = Year(Now)

    Dim strErrorMessage As String

        passedValidation = lngYear >= 2000 And lngYear <= currentYear

        If Not passedValidation Then
            strErrorMessage = "Please select a valid year"
            PrintErrorMessage strErrorMessage
        End If

End Sub

Private Sub ValidateCellSelectionBox(ByRef cellSelectionBox As RefEdit.RefEdit)

    Dim cellAddress As String
        cellAddress = cellSelectionBox.Text

    Dim passedValidation As Boolean
        passedValidation = False

    Dim testRange As Variant
    Set testRange = Range(cellAddress)

    Dim strErrorMessage As String

        passedValidation = TypeName(testRange) = "Range" And testRange.Count = 1

        If Not passedValidation Then
            strErrorMessage = "Please select a valid cell address"
            PrintErrorMessage strErrorMessage
        End If

End Sub
\$\endgroup\$
3
  • 1
    \$\begingroup\$ I'd be thinking - where are the comments? \$\endgroup\$ Commented Dec 15, 2015 at 16:53
  • 1
    \$\begingroup\$ The longest procedure is 17 lines of instructions. I *hope* the code should document itself. \$\endgroup\$
    – Kaz
    Commented Dec 15, 2015 at 17:00
  • 1
    \$\begingroup\$ I agree with @Raystafarian, there's still room to put some comments on a logical level. Also I'd put a short descriptive comment for each of the functions, and their expected inputs/outputs. The code isn't completely self descriptive. \$\endgroup\$ Commented Dec 15, 2015 at 17:12

1 Answer 1

3
\$\begingroup\$

This is going to make for either a poor UI or a change in behavior later. Better to just change its behavior now.

      Dim ixYear As Long

      For ixYear = 2000 To Year(Now)
           yearBox.AddItem ixYear
      Next ixYear

The problem is that this is always and forever going to start on the year 2000. The list will just continue to grow over time. That's fine if you need the year 2000 to always be there, but you should ask your users if that's the case. I see two options here.

  1. Calculate backwards 15 years.

    Dim thisYear = Year(Now)
    For ixYear = (thisYear - 15) To thisYear
    
  2. Make this year the first option in the list and populate the list in reverse.

    For ixYear = Year(Now) To 2000 Step -1
    

The approach you take will depend on your exact requirements. Of course, if you take the second route, you'll quickly find all the places where you hardcoded the number 2000.

The only other "issue" I see worth mentioning is this use of empty quotes.

  If yearBox.Text <> "" Then

You should use vbNullString wherever possible. It's intent is more clear and it uses less memory than the empty string literal. (Okay, so it's a negligible amount of memory, but still...)

All in all its good code, very self documenting as far as what it does, but some comments explaining why couldn't hurt. For example, why does the selection start at the year 2000?

I've got to say, I'm impressed at how far you've come since your first post here.

\$\endgroup\$
2
  • \$\begingroup\$ Honestly, 2000 is just a placeholder. The actual start of the data is much more recent than that. Good shout on populating in reverse order. Also, would you believe it's only been 3 months :p \$\endgroup\$
    – Kaz
    Commented Dec 17, 2015 at 1:21
  • \$\begingroup\$ That is hard to believe. Pretty soon you'll be discovering model view presenter. Then you'll start abusing the language until you just give up and move on to a language a bit newer. =;)- \$\endgroup\$
    – RubberDuck
    Commented Dec 17, 2015 at 1:29

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.