patternMinor
UserForm to handle Date Inputs (Day,Month,Year)
Viewed 0 times
inputsyearhandleuserformdatemonthday
Problem
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
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)
dayBo
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)
dayBo
Solution
This is going to make for either a poor UI or a change in behavior later. Better to just change its behavior now.
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.
-
Calculate backwards 15 years.
-
Make this year the first option in the list and populate the list in reverse.
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
The only other "issue" I see worth mentioning is this use of empty quotes.
You should use
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.
Dim ixYear As Long
For ixYear = 2000 To Year(Now)
yearBox.AddItem ixYear
Next ixYearThe 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.
-
Calculate backwards 15 years.
Dim thisYear = Year(Now)
For ixYear = (thisYear - 15) To thisYear-
Make this year the first option in the list and populate the list in reverse.
For ixYear = Year(Now) To 2000 Step -1The 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 <> "" ThenYou 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.
Code Snippets
Dim ixYear As Long
For ixYear = 2000 To Year(Now)
yearBox.AddItem ixYear
Next ixYearDim thisYear = Year(Now)
For ixYear = (thisYear - 15) To thisYearFor ixYear = Year(Now) To 2000 Step -1If yearBox.Text <> "" ThenContext
StackExchange Code Review Q#114051, answer score: 3
Revisions (0)
No revisions yet.