7
\$\begingroup\$

I'm trying to avoid strings being passed around as a valid value. The ability to make a mistake is too high for my taste and the solution that I've come up with is to use an Enum. This provides the benefit of intellisense when trying to use the values. Similar to typing sheet1.Visible = which gives the valid options a worksheet property with its type declared As SimpleSpanBracing allows the valid values be chosen. Now when I need to use something or to Get or Let a value I use the converter to ensure that I'm not going to mistakenly mix up casing, ends only or midspan, and accidentally get a false positive/negative.

Is there anything that I'm missing or haven't guarded against?

Enumeration with valid options. notSet is an option because I don't want to assume the default value. I'd prefer to see what's causing the issue and how it came about.

Public Enum SimpleSpanBracing
    notSet
    endsonly
    midspan
    thirdspan
End Enum

SpanBracingConverter Class that does the actual converting. I'm not concerned with getting a value that hasn't been set, but setting it to an invalid issues will cause an error to be raised, avoiding a false positive/negative mentioned above.

Private Const spanEnd As String = "ENDS ONLY"
Private Const spanMid As String = "MIDSPAN"
Private Const spanThird As String = "1/3 SPAN"
Private Const INVALID_INPUT As Long = 5

Public Function ToString(ByVal value As SimpleSpanBracing) As String
    If value = endsonly Then
        ToString = spanEnd
    ElseIf value = midspan Then
        ToString = spanMid
    ElseIf value = thirdspan Then
        ToString = spanThird
    Else
        Err.Raise INVALID_INPUT, "SpanBracingConverter.ToString()", "Invalid input supplied"
    End If
End Function
Public Function ToEnum(ByVal value As String) As SimpleSpanBracing
    value = UCase$(value)
    If value = spanEnd Then
        ToEnum = endsonly
    ElseIf value = spanMid Then
        ToEnum = midspan
    ElseIf value = spanThird Then
        ToEnum = thirdspan
    Else
        Err.Raise INVALID_INPUT, "SpanBracingConverter.ToEnum()", "Invalid input supplied"
    End If
End Function
Public Function ValidateString(ByVal value As String) As String
    ValidateString = ToString(ToEnum(value))
End Function

What follows are the unit tests I've created with Rubberduck to test this converter.

'@TestMethod
Public Sub ValidInputs()
    On Error GoTo TestFail

    'Arrange:
    Dim sut As SpanBracingConverter
    Set sut = New SpanBracingConverter

    'Act:
    Dim lowerEnd As SimpleSpanBracing
    lowerEnd = sut.ToEnum("ends only")
    Dim upperEnd As SimpleSpanBracing
    upperEnd = sut.ToEnum("ENDS ONLY")
    Dim lowerMid As SimpleSpanBracing
    lowerMid = sut.ToEnum("midspan")
    Dim upperMid As SimpleSpanBracing
    upperMid = sut.ToEnum("MIDSPAN")
    Dim lowerThird As SimpleSpanBracing
    lowerThird = sut.ToEnum("1/3 span")
    Dim upperThird As SimpleSpanBracing
    upperThird = sut.ToEnum("1/3 SPAN")

    Dim endInput As String
    endInput = sut.ToString(endsonly)
    Dim midInput As String
    midInput = sut.ToString(midspan)
    Dim thirdInput As String
    thirdInput = sut.ToString(thirdspan)

    'Assert:
    Assert.areequal SimpleSpanBracing.endsonly, lowerEnd
    Assert.areequal SimpleSpanBracing.endsonly, upperEnd
    Assert.areequal SimpleSpanBracing.midspan, lowerMid
    Assert.areequal SimpleSpanBracing.midspan, upperMid
    Assert.areequal SimpleSpanBracing.thirdspan, lowerThird
    Assert.areequal SimpleSpanBracing.thirdspan, upperThird

    Assert.areequal "ENDS ONLY", endInput
    Assert.areequal "MIDSPAN", midInput
    Assert.areequal "1/3 SPAN", thirdInput

TestExit:
    Exit Sub
TestFail:
    Assert.Fail "Test raised an error: #" & Err.Number & " - " & Err.Description
End Sub

'@TestMethod
Public Sub InvalidStringInput()
    Const ExpectedError As Long = 5
    On Error GoTo TestFail

    'Arrange:

    'Act:
    With New SpanBracingConverter
        .ToEnum "fail"
    End With

Assert:
    Assert.Fail "Expected error was not raised."

TestExit:
    Exit Sub
TestFail:
    If Err.Number = ExpectedError Then
        Resume TestExit
    Else
        Resume Assert
    End If
End Sub

'@TestMethod
Public Sub UnsetEnumValue()
    Const ExpectedError As Long = 5
    On Error GoTo TestFail

    'Arrange:

    'Act:
    With New SpanBracingConverter
        .ToString SimpleSpanBracing.notSet
    End With

TestExit:
    Exit Sub
TestFail:
    If Err.Number = ExpectedError Then
        Resume TestExit
    Else
        Assert.Fail "Expected error was not raised."
    End If
End Sub

'@TestMethod
Public Sub LargestAssignedEnumValue()
    Const ExpectedError As Long = 5
    On Error GoTo TestFail

    'Arrange:

    'Act:
    With New SpanBracingConverter
        .ToString 4
    End With

Assert:
    Assert.Fail "Expected error was not raised."

TestExit:
    Exit Sub
TestFail:
    If Err.Number = ExpectedError Then
        Resume TestExit
    Else
        Assert.Fail "Expected error was not raised."
    End If
End Sub

'@TestMethod
Public Sub InputMatchesOutputCaseInsensitive()
    On Error GoTo TestFail

    'Arrange:

    'Act:

    'Assert:
    With New SpanBracingConverter
        Assert.areequal "ENDS ONLY", .ValidateString("ENDS ONLY")
        Assert.areequal "ENDS ONLY", .ValidateString("ends only")
        Assert.areequal "MIDSPAN", .ValidateString("MIDSPAN")
        Assert.areequal "MIDSPAN", .ValidateString("midspan")
        Assert.areequal "1/3 SPAN", .ValidateString("1/3 SPAN")
        Assert.areequal "1/3 SPAN", .ValidateString("1/3 span")
    End With

TestExit:
    Exit Sub
TestFail:
    Assert.Fail "Test raised an error: #" & Err.Number & " - " & Err.Description
End Sub

Edit: My followup question, Part 2 continues my work on the converter class.

\$\endgroup\$
0

1 Answer 1

Reset to default
3
\$\begingroup\$

The Enum

An Enum is essentially a type, it has members. Members should be PascalCase - I'd change the alllowercase casing to respect that convention:

Public Enum SimpleSpanBracing
    NotSet = 0
    EndsOnly
    MidSpan
    ThirdSpan
End Enum

I would also make the NotSet value explicitly 0, because otherwise someone could very well think the enum would look prettier if all its members were listed alphabetically, and that would royally mess things up.

The Converter

The job of that class is essentially to map strings to enum values, and enum values to strings. Whenever I use the word "map", I automatically think of a Dictionary.

Picture your enum with 200 members. I doubt you find that If...ElseIf...ElseIf... conditional structure nice & neat now, huh?

A Select Case...Case...Case... block would be better IMO, but still rather annoying to maintain, given you essentially need the same blocks twice.

I'd go with a Private Dictionary - keyed with the strings and valued with the enums.

Then I'd have another Private Dictionary, keyed with [a string representation of] the enum values, and valued with the corresponding strings.

In the class' Initialize handler, I'd populate one of them, and then iterate it to automatically populate the other; of course the two methods would be implemented in their own dedicated Private Sub procedures, and the string constants would no longer serve a purpose.

Then, ToString and ToEnum would end up being as straightforward as it gets:

Public Function ToString(ByVal value As SimpleSpanBracing) As String
    If Not StringForEnum.Exists(CStr(value)) Then
        ThrowInvalidArgument "ToString", CStr(value)
        'Exit Function
    End If
    ToString = StringForEnum(CStr(value))
End Function

Public Function ToEnum(ByVal value As String) As SimpleSpanBracing
    If Not EnumForString.Exists(value) Then
        ThrowInvalidArgument "ToEnum", value
        'Exit Function
    End If
    ToEnum = EnumForString(value)
End Function

I'd have a dedicated ThrowInvalidArgument procedure responsible for throwing the invalid argument error, and move the INVALID_INPUT declaration there... or just remove it and inline the message string.

Private Sub ThrowInvalidArgument(ByVal source As String, ByVal value As String)
    Err.Raise 5, TypeName(Me) & "." & source, _
        "Invalid input '" & value & "' was supplied."
End Sub

The purpose of ValidateString doesn't strike me as super-obvious just by the name; my brain somehow expects "ValidateFoo" to either return a Boolean telling me whether I've supplied a valid Foo, or to throw an error given an invalid value. That method is really just a "round-trip" utility function that, IMO, should be inlined at call sites.

As a bonus of using that dictionary approach, you could expose a Property Get member that returns an array containing all the valid enum values, and another that returns all the valid strings. More on that in a minute.

The Tests

IMO all the Dim statements are cluttering the "Act" part of the tests and making it harder than necessary to read the actual "acting" executable statements.

Rubberduck's IAssert interface exposes a convenient SequenceEquals method that you can use to iterate and compare array elements; instead of making a series of Assert.AreEqual calls, you could have the converter expose a Property Get member that gives you all the valid strings as an array, a Property Get member that gives you all the valid enum values as another array, and then iterate one array to populate a "result" array by invoking your SUT/conversion in one single place (in that loop) - and then make one single Assert call to compare the expected vs. the actual resulting array.

\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.