Reducing Cyclomatic Complexity in a select

Question

Good day coding godz,

I am trying to clean up some code and was wondering how to reduce the Cylcomatic Complexity on a method I have created. This method is used during an Import of a CSV file. One of the fields in the CSV file is a license type that is a string (ie "BFI Supervised Lender - Website #2 License") I am converting this to a Integer (1,2,3 or 4) that will be saved to the Database to reference the type of industry based on license type.

Below is my method. Would appreciate some tips for a shade tree VB.NET coder...

Private Function CheckIndustryType(LicName)
    Dim VAR_IndType As Integer

    Select Case Text_LicName
        Case "BFI Level I Check Cashing - Branch License"
            VAR_IndType = 3
        Case "BFI Level II Check Cashing - Branch Certificate"
            VAR_IndType = 3
        Case "BFI Supervised Lender - Branch License"
            VAR_IndType = 1
        Case "BFI Deferred Presentment - Branch License"
            VAR_IndType = 3
        Case "BFI Supervised Lender - Website #1 License"
            VAR_IndType = 1
        Case "BFI Supervised Lender - Website #2 License"
            VAR_IndType = 1
        Case "BFI Supervised Lender - Website #3 License"
            VAR_IndType = 1
        Case "BFI Supervised Lender - Website #4 License"
            VAR_IndType = 1
        Case "BFI Supervised Lender - Website #5 License"
            VAR_IndType = 1
        Case "BFI Supervised Lender - Website #6 License"
            VAR_IndType = 1
        Case "BFI Level II Check Cashing - Company License"
            VAR_IndType = 3
        Case "BFI Level I Check Cashing - Company License"
            VAR_IndType = 3
        Case "fI Branch Mortgage Lender/Servicer"
            VAR_IndType = 2
        Case "BFI Branch Mortgage Lender/Servicer - Other Trade Name #1"
            VAR_IndType = 2
        Case "BFI Branch Mortgage Lender/Servicer - Other Trade Name #2"
            VAR_IndType = 2
        Case "BFI Branch Mortgage Lender/Servicer - Other Trade Name #3"
            VAR_IndType = 2
        Case "BFI Branch Mortgage Lender/Servicer - Other Trade Name #4"
            VAR_IndType = 2
        Case "BFI Branch Mortgage Lender/Servicer - Other Trade Name #5"
            VAR_IndType = 2
        Case "BFI Branch Mortgage Lender/Servicer - Other Trade Name #6"
            VAR_IndType = 2
        Case "BFI Mortgage Lender / Servicer License"
            VAR_IndType = 2
        Case "BFI Mortgage Lender/Servicer License - Other Trade Name #1"
            VAR_IndType = 2
        Case "BFI Mortgage Lender/Servicer License - Other Trade Name #2"
            VAR_IndType = 2
        Case "BFI Mortgage Lender/Servicer License - Other Trade Name #3"
            VAR_IndType = 2
        Case "BFI Mortgage Lender/Servicer License - Other Trade Name #4"
            VAR_IndType = 2
        Case "BFI Mortgage Lender/Servicer License - Other Trade Name #5"
            VAR_IndType = 2
        Case "BFI Mortgage Lender/Servicer License - Other Trade Name #6"
            VAR_IndType = 2
        Case Else
            VAR_IndType = 4
    End Select

    Return VAR_IndType 
End Function

I'd avoid making any changes (unless you can automate them). You're likely to introduce at least one bug due to a typing error. You've already got this encapsulated into a function. (Also: I'd be a little suspicious of the calculated complexity; it's probably based on this being turned into an if-then-else tree behind-the-scenes, but I don't think it's an accurate reflection of the actual mental load of processing the function.) — Craig, Nov 29 '21 at 16:15
@Craig In some circumstances, I have read that a Select Case can be turned into a lookup table. It happens in C# with strings, but I can't find a definitive article on the subject. (And the optimisation is not performed in debug mode.) — Andrew Morton, Nov 29 '21 at 18:18
If you *do* refactor this, make sure you've got extensive unit tests in place so that you can have total confidence in catching any errors you introduce. — Craig, Nov 29 '21 at 18:22

jonyfries · Accepted Answer · 2021-11-30T12:26:05.397

I would use a dictionary in a situation like this. It reduces the number of independent paths to 2, the second is necessary since you're using a default value of course.

Module Module1
    Dim industryTypeMap As New Dictionary(Of String, Int32) From {{"BFI Level I Check Cashing - Branch License", 3},
                                                                           {"BFI Level II Check Cashing - Branch Certificate", 3},
                                                                           {"BFI Supervised Lender - Branch License", 1}}

    Sub Main()
        Console.WriteLine(CheckIndustryType("BFI Supervised Lender - Branch License"))
        Console.WriteLine(CheckIndustryType("BFI Level II Check Cashing - Branch Certificate"))
        Console.WriteLine(CheckIndustryType("Other"))
    End Sub

    Private Function CheckIndustryType(LicName As String)
        Dim industryInt As Int32
        If industryTypeMap.TryGetValue(LicName, industryInt) Then
            Return industryInt
        Else
            Return 4
        End If
    End Function
End Module

This outputs:

1
3
4

You could also define the dictionary in the function to maintain all of the code together but that would obviously run slower if you're calling the function repeatedly.

As per the comment below - Ideally you would place the actual mapping in an external item that could be updated without recompiling the code (eg a config file or a database).

It would be a good idea to put the number<-string mapping in an external file so that if something changes then the program doesn't need to be recompiled. — Andrew Morton, Nov 29 '21 at 18:19
Very good point @AndrewMorton - A database or a config file should really be used for this if it might ever change. And anything in business might someday change. — jonyfries, Nov 30 '21 at 12:22

score 0 · Answer 2 · answered Nov 29 '21 at 17:44

0

I shortened the number of cases by using StartsWith and Contains with Select Case True.

Private Function CheckIndustryType(LicName As String) As Integer
    Dim VAR_IndType As Integer

    Select Case True
        Case LicName.Contains("Check Cashing")
            VAR_IndType = 3
        Case LicName.StartsWith("BFI Supervised Lender")
            VAR_IndType = 1
        Case LicName.StartsWith("BFI Deferred Presentment")
            VAR_IndType = 3
        Case LicName.StartsWith("fI")
            VAR_IndType = 2
        Case LicName.StartsWith("BFI Branch Mortgage Lender")
            VAR_IndType = 2
        Case Else
            VAR_IndType = 4
    End Select

    Return VAR_IndType
End Function

answered Nov 29 '21 at 17:44

Mary

14,926
3
18
27

`Select Case True` doesn't really do anything. Seems better in such a case just to use a sequence of `If` statements. – Joshua Frank Nov 29 '21 at 17:48
@JoshuaFrank I wouldn't agree that it doesn't do anything. It leverages the implicit transformation into an `If`-`Then`-`Else` tree, and it's potentially preferable in this case because it makes the logical structure more obvious (at the cost of the mental load to figure out what it's doing if you haven't seen the idiom before). – Craig Nov 29 '21 at 18:21
@JoshuaFrank What do you mean "doesn't really do anything"? – Mary Nov 29 '21 at 18:53
@Mary: I just meant that in `Select Case True`, True is always True, so it's a somewhat confusing idiom, compared to writing If condition else if condition2 else if condition 3. But as the OP says, a long list of tests is undesirable to begin with, so coding it slightly differently is probably not the best outcome anyway. – Joshua Frank Nov 29 '21 at 19:25

score 0 · Answer 3 · answered Nov 29 '21 at 18:03

A long list of long strings is risky, because any change is quite likely to introduce a typo and then the strings don't match. I would probably make an Enum:

Enum LicenseType
  BFILevelICheckCashingBranchLicense
  BFILevelIICheckCashingBranchCertificate
  BFISupervisedLenderBranchLicense
  ...
End Enum

and then test against that value. To reduce the complexity, I would make an attribute like:

Class VarIndTypeAttribute 
  Inherits System.Attribute

    Public VarIndType As Integer
    Sub New(varIndType As Integer
        Me.VarIndType = varIndType
    End Sub

End Class

and then the enum becomes:

Enum
  <VarIndType(3)>
  BFILevelICheckCashingBranchLicense
  <VarIndType(3)>
  BFILevelIICheckCashingBranchCertificate
  <VarIndType(1)>
  BFISupervisedLenderBranchLicense
  ...
End Enum

and then you make a method like

    <Extension>
    Function ToVarIndType(enumValue As [Enum]) As Integer
        Dim att = enumValue.GetAttributeOfType(Of VarIndTypeAttribute)()
        Return If(att IsNot Nothing, att.VarIndType, 0)
    End Function

and then

    Dim valueEnum As LicenseType = ...
    Dim VarIndType = valueEnum.ToVarIndType

and you don't need the lookup function at all!

You probably want VarIndType to be an Enum as well, so you don't have magic values like 0 and 3 throughout the app.

Reducing Cyclomatic Complexity in a select

3 Answers3