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
\$\endgroup\$
8
\$\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.

\$\endgroup\$
  • 1
    \$\begingroup\$ This answer is awesome. I'll definitely be taking it to heart. \$\endgroup\$ – Kaz Mar 31 '16 at 9:09

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.