Part of my standard modules deal with my company's employees and linking them to/from various data sets.
The following are a series of enums/functions to handle peoples' names, namely an Enum
for advisers and functions to:
- Provide a collection of names associated to each adviser (e.g. their name before/after getting married)
- Provide the current name of each adviser
- Determine which adviser's name appears in a given string
Is this a good, maintainable, extensible system? Especially considering the possibility of new employees over time?
Option Explicit
Public Enum LuminAdviser
MartinCotter = 1
JonHussey = 2
JeremySmith = 3
JonathanBlair = 4
JohnCusins = 5
SarahCotter = 6
MickyMahbubani = 7
End Enum
Public Function NameStringsFromAdviser(ByVal adviser As LuminAdviser) As Collection
Dim coll As Collection
Set coll = New Collection
Select Case adviser
Case JonHussey
AddNameVariations coll, "Jon", "Hussey"
Case MartinCotter
AddNameVariations coll, "Martin", "Cotter"
Case JeremySmith
AddNameVariations coll, "Jeremy", "Smith"
Case JonathanBlair
AddNameVariations coll, "Jonathan", "Blair"
Case SarahCotter
AddNameVariations coll, "Sarah", "Cotter"
AddNameVariations coll, "Sarah", "Oluwole"
Case JohnCusins
AddNameVariations coll, "John", "Cusins"
Case MickyMahbubani
AddNameVariations coll, "Micky", "Mahbubani"
End Select
Set NameStringsCollectionFromAdviser = coll
End Function
Public Sub AddNameVariations(ByRef coll As Collection, ByVal forename As String, ByVal surname As String)
coll.Add forename & " " & surname
coll.Add surname & ", " & forename
End Sub
Public Function CurrentNameStringFromAdviser(ByVal adviser As LuminAdviser) As String
Dim nameString As String
Select Case adviser
Case JonHussey
nameString = "Hussey, Jon"
Case MartinCotter
nameString = "Cotter, Martin"
Case JeremySmith
nameString = "Smith, Jeremy"
Case JonathanBlair
nameString = "Blair, Jonathan"
Case SarahCotter
nameString = "Oluwole, Sarah"
Case JohnCusins
nameString = "Cusins, John"
Case MickyMahbubani
nameString = "Mahbubani, Micky"
End Select
CurrentNameStringFromAdviser = nameString
End Function
Public Function AdviserFromString(ByVal str As String) As LuminAdviser
Dim foundAdviser As Boolean
foundAdviser = True
Dim adviser As LuminAdviser
If AdviserNameInString(JeremySmith, str) Then
adviser = JeremySmith
ElseIf AdviserNameInString(JohnCusins, str) Then
adviser = JohnCusins
ElseIf AdviserNameInString(JonathanBlair, str) Then
adviser = JonathanBlair
ElseIf AdviserNameInString(JonHussey, str) Then
adviser = JonHussey
ElseIf AdviserNameInString(MartinCotter, str) Then
adviser = MartinCotter
ElseIf AdviserNameInString(MickyMahbubani, str) Then
adviser = MickyMahbubani
ElseIf AdviserNameInString(SarahCotter, str) Then
adviser = SarahCotter
Else
foundAdviser = False
End If
If Not foundAdviser Then PrintErrorMessage "Could not find any adviser in string: " & str
AdviserFromString = adviser
End Function
Public Function AdviserNameInString(ByVal adviser As LuminAdviser, ByVal str As String) As Boolean
Dim foundAdviser As Boolean
foundAdviser = False
Dim nameColl As Collection
nameColl = NameStringsFromAdviser(adviser)
Dim i As Long
For i = 1 To nameColl.Count
If InStr(1, str, nameColl(i)) > 0 Then
foundAdviser = True
Exit For
End If
Next i
AdviserNameInString = foundAdviser
End Function
1 Answer 1
First, I thought I'd mention that your code is stylistically really good - variable and function names make sense, indentation is great (with one small exception noted below), and the functions are focused and readable. Just one minor point:
The code under each Case
in a Select Case
is typically indented one more level:
Select Case adviser
Case JonHussey
AddNameVariations coll, "Jon", "Hussey"
Case MartinCotter
'...
As far as your specific question goes, this actually looks like it would be quite a chore to maintain. Adding a new name to an existing advisor is easy with your set-up. For example, if John Cusins decided to change his name to Jim Nasium, all you would have to do is add AddNameVariations coll, "Jim", "Nasium"
under his Case
in NameStringsFromAdviser
and change his Case
in CurrentNameStringFromAdviser
to NameString = "Nasium, Jim"
.
But, this points to the next issue - your Enum
isn't really serving much use as much other than a place-holder. On the one hand you could change the JohnCusins
element to JimNasium
everywhere in your code, but this kind of defeats the purpose of having a named identifier. On the other hand you could leave it as JohnCusins
, but this now means that it isn't appropriately named - why should the Enum
element associated with a person be different from their name? If I was only looking at the CurrentNameStringFromAdviser
function and not the entire module, I'd be wondering "why is the SarahCotter case returning Oluwole, Sarah
?"
This also points to the main issue in maintainability and extensibility - to add an advisor, you need to change the code in 3 different functions and add a value to the enumeration. At the same time, you need to make sure that those changes are all consistent with each other. The urge to copy and paste will be very hard to resist...
...which leads to the last point - you're repeating code all over the place. This gives me a whiff of code smell. Wouldn't it be nice if you didn't have to type If AdviserNameInString(FooBar, str) Then
again and again and again? It would be much better to approach this problem with the mantra Don't Repeat Yourself in mind. This is a problem begging for an OO solution.
You have 2 distinct things you care about - advisors and names. So make an advisor class and a name class. Make them responsible for the things that advisors and names care about. I.e.:
Name.cls
Option Explicit
Private mFirst As String
Private mLast As String
Public Property Let FirstName(first As String)
mFirst = first
End Property
Public Property Get FirstName() As String
FirstName = mFirst
End Property
Public Property Let LastName(last As String)
mLast = last
End Property
Public Property Get LastName() As String
LastName = mLast
End Property
Public Property Get NameString() As String
NameString = mLast & ", " & mFirst
End Property
Public Function Matches(test As String) As Boolean
Matches = InStr(1, test, mFirst) Or InStr(1, test, mLast)
End Function
Advisor.cls:
Option Explicit
Private mNames As New Collection
Private mPreferred As Name
Public Sub AddName(inValue As Name)
mNames.Add inValue
End Sub
Public Property Set PreferredName(inValue As Name)
Set mPreferred = inValue
mNames.Add inValue
End Property
Public Property Get PreferredName() As Name
Set PreferredName = mPreferred
End Property
Public Function NameMatches(test As String) As Boolean
Dim candidate As Variant
For Each candidate In mNames
If candidate.Matches(test) Then
NameMatches = True
Exit For
End If
Next candidate
End Function
Note, it isn't clear from your question if the numbers in the Enum
have meaning outside of the code that you posted. If they are, just make them a property of Advisor. Throw in a couple factory functions to easily generate objects (curse you VBA for not having constructors)...
'Name factory.
Public Function NewName(first As String, last As String) As Name
Set NewName = New Name
NewName.FirstName = first
NewName.LastName = last
End Function
'Advisor factory
Public Function NewAdvisor(preferred As Name, _
ParamArray names() As Variant) As Advisor
Dim variation As Variant
Set NewAdvisor = New Advisor
Set NewAdvisor.PreferredName = preferred
For Each variation In names
Dim cast As Name
Set cast = variation
NewAdvisor.AddName cast
Next variation
End Function
...and you get something to maintain that looks like this:
Option Explicit
Private Advisors As Collection
Public Sub InitializeAdvisors()
Set Advisors = New Collection
Advisors.Add NewAdvisor(NewName("Jon", "Hussey"))
Advisors.Add NewAdvisor(NewName("Martin", "Cotter"))
Advisors.Add NewAdvisor(NewName("Jeremy", "Smith"))
Advisors.Add NewAdvisor(NewName("Jonathan", "Blair"))
Advisors.Add NewAdvisor(NewName("Sarah", "Oluwole"), _
NewName("Sarah", "Cotter"))
Advisors.Add NewAdvisor(NewName("John", "Cusins"))
Advisors.Add NewAdvisor(NewName("Micky", "Mahbubani"))
End Sub
Public Function FindAdvisor(inValue As String) As Advisor
Dim candidate As Variant
For Each candidate In Advisors
If candidate.NameMatches(inValue) Then
Set FindAdvisor = candidate
Exit Function
End If
Next candidate
End Function
Example usage:
Private Sub ExampleUsage()
InitializeAdvisors
Debug.Print FindAdvisor("Someone named Sarah").PreferredName().NameString
End Sub
Add an advisor? One line of code. Add an alias? Change one line of code. Remove an advisor? Delete one line of code. Change from "Last, First" format to "First Last" format? Change one line of code. An advisor has 42 aliases? No problem - just add them all in one place. Now that is easy to maintain.
-
1\$\begingroup\$ This answer is awesome. I'll definitely be taking it to heart. \$\endgroup\$Kaz– Kaz2016年03月31日 09:09:53 +00:00Commented Mar 31, 2016 at 9:09