3
\$\begingroup\$

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
Jamal
35.2k13 gold badges134 silver badges238 bronze badges
asked Mar 30, 2016 at 10:10
\$\endgroup\$

1 Answer 1

9
\$\begingroup\$

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.

answered Mar 31, 2016 at 1:39
\$\endgroup\$
1
  • 1
    \$\begingroup\$ This answer is awesome. I'll definitely be taking it to heart. \$\endgroup\$ Commented Mar 31, 2016 at 9:09

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

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

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.