HiveBrain v1.2.0
Get Started
← Back to all entries
patternMinor

Handling name variants

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
codereviewstackoverflowvba
handlingnamevariants

Problem

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 Ma

Solution

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 Ne

Code Snippets

Select Case adviser
    Case JonHussey
        AddNameVariations coll, "Jon", "Hussey"
    Case MartinCotter
        '...
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
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
'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
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

Context

StackExchange Code Review Q#124253, answer score: 9

Revisions (0)

No revisions yet.